[PATCH] fwts:dmicheck: replace memcpy with fwts_memcpy_unaligned for SMBIOS 3.0 table
Colin Ian King
colin.king at canonical.com
Tue Nov 29 21:10:32 UTC 2016
On 29/11/16 20:37, wufan wrote:
> From f507dd17853f7e17cecfa2d32d28448913c60873 Mon Sep 17 00:00:00 2001
> From: Fan Wu <wufan at codeaurora.org>
> Date: Tue, 29 Nov 2016 20:28:13 +0000
> Subject: [PATCH] fwts:dmicheck: replace memcpy with fwts_memcpy_unaligned
> for
> SMBIOS 3.0 table
>
> Mapping of SMBIOS 3.0 table in AARCH64 platforms through /dev/mem
> has the memory tagged as device memory, and memcpy could trigger
> alignment fault if the SMBIOS table is of odd size. The fix is to
> replace memcpy with fwts_memcpy_unaligned which does byte copy.
> ---
> src/dmi/dmicheck/dmicheck.c | 4 ++--
> src/lib/include/fwts_stringextras.h | 1 +
> src/lib/src/fwts_stringextras.c | 16 ++++++++++++++++
> 3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/src/dmi/dmicheck/dmicheck.c b/src/dmi/dmicheck/dmicheck.c
> index 1934ebe..62eef9c 100644
> --- a/src/dmi/dmicheck/dmicheck.c
> +++ b/src/dmi/dmicheck/dmicheck.c
> @@ -370,8 +370,8 @@ static void* dmi_table_smbios30(fwts_framework *fw,
> fwts_smbios30_entry *entry)
> mem = fwts_mmap(addr, length);
> if (mem != FWTS_MAP_FAILED) {
> table = malloc(length);
> - if (table)
> - memcpy(table, mem, length);
> + if (table)
> + fwts_memcpy_unaligned(table, mem, length);
> (void)fwts_munmap(mem, length);
> return table;
> }
> diff --git a/src/lib/include/fwts_stringextras.h
> b/src/lib/include/fwts_stringextras.h
> index 6256254..b364127 100644
> --- a/src/lib/include/fwts_stringextras.h
> +++ b/src/lib/include/fwts_stringextras.h
> @@ -25,5 +25,6 @@
> void fwts_chop_newline(char *str);
> char *fwts_realloc_strcat(char *orig, const char *newstr);
> char *fwts_string_endswith(const char *str, const char *postfix);
> +void fwts_memcpy_unaligned(void *dst, const void *src, size_t n);
>
> #endif
> diff --git a/src/lib/src/fwts_stringextras.c
> b/src/lib/src/fwts_stringextras.c
> index 17d833e..25465a8 100644
> --- a/src/lib/src/fwts_stringextras.c
> +++ b/src/lib/src/fwts_stringextras.c
> @@ -92,3 +92,19 @@ char* fwts_string_endswith(const char *str, const char
> *postfix)
>
> return (char*) str + sl - pl;
> }
> +
> +
> +/*
> + * fwts_memcpy_unaligned()
> + * perform byte copy of n bytes from src to dst.
> + */
> +void fwts_memcpy_unaligned(void *dst, const void *src, size_t n)
> +{
> + size_t i = n;
> +
> + if (dst == NULL || src == NULL || n == 0 )
> + return;
> +
> + while (i--)
> + ((uint8_t*)dst)[i] = ((uint8_t*)src)[i];
> +}
> --
> 2.7.4
>
> -----Original Message-----
> From: Colin Ian King [mailto:colin.king at canonical.com]
> Sent: Monday, November 28, 2016 1:50 PM
> To: wufan <wufan at codeaurora.org>; fwts-devel at lists.ubuntu.com
> Subject: Re: [PATCH] fwts:dmicheck: replace memcpy with byte copy for SMBIOS
> 3.0 table
>
> On 28/11/16 20:12, wufan wrote:
>> This is a real issue. Without the patch the dmicheck test will fail
>> with
>> segfault:
>>
>> ubuntu at ubuntu:~/fwts/src$ sudo ./fwts dmicheck - Results generated by
>> fwts: Version V16.11.00 (2016-11-10 08:39:49).
>>
>> Some of this work - Copyright (c) 1999 - 2016, Intel Corp. All rights
>> reserved.
>> Some of this work - Copyright (c) 2010 - 2016, Canonical.
>> Some of this work - Copyright (c) 2016 IBM.
>>
>> This test run on 28/11/16 at 18:56:40 on host Linux ubuntu 4.7.0 #2
>> SMP Sun Oct 16 14:57:06 MDT 2016 aarch64.
>>
>> Command: "fwts dmicheck -".
>> Running tests: dmicheck.
>>
>> dmicheck: DMI/SMBIOS table tests.
>> ----------------------------------------------------------------------
>> ------
>> ----------------------------------------------------
>> Test 1 of 3: Find and test SMBIOS Table Entry Points.
>> This test tries to find and sanity check the SMBIOS data structures.
>> PASSED: Test 1, Found SMBIOS30 Table Entry Point at 0x43fffb0000
>> SMBIOS30 Entry Point Structure:
>> Anchor String : _SM3_
>> Checksum : 0x9b
>> Entry Point Length : 0x18
>> Major Version : 0x03
>> Minor Version : 0x00
>> Docrev : 0x00
>> Entry Point Revision : 0x01
>> Reserved : 0x00
>> Table maximum size : 0x0000263d
>> Table address : 0x00000043f9190000
>>
>> PASSED: Test 1, SMBIOS30 Table Entry Point Checksum is valid.
>> PASSED: Test 1, SMBIOS30 Table Entry Point Length is valid.
>>
>> Caught SIGNAL 11 (Segmentation fault), aborting.
>> Backtrace:
>> 0x0000ffffac10ff84
>> /home/ubuntu/fwts/src/lib/src/.libs/libfwts.so.1(+0xff84)
>>
>>
>> Fan
>>
>>
>> -----Original Message-----
>> From: Colin Ian King [mailto:colin.king at canonical.com]
>> Sent: Monday, November 28, 2016 1:08 PM
>> To: wufan <wufan at codeaurora.org>; fwts-devel at lists.ubuntu.com
>> Subject: Re: [PATCH] fwts:dmicheck: replace memcpy with byte copy for
>> SMBIOS
>> 3.0 table
>>
>> On 28/11/16 18:26, wufan wrote:
>>> From 00cae446405d4d856fbd56222f835c207d18d8e8 Mon Sep 17 00:00:00
>>> 2001
>>>
>>> From: Fan Wu <wufan at codeaurora.org>
>>>
>>> Date: Thu, 3 Nov 2016 14:03:40 -0600
>>>
>>> Subject: [PATCH] fwts:dmicheck: replace memcpy with byte copy for
>>> SMBIOS 3.0
>>>
>>> table
>>>
>>>
>>>
>>> Mapping of SMBIOS 3.0 table in AARCH64 platforms through /dev/mem
>>>
>>> has the memory tagged as device memory, and memcpy could trigger
>>>
>>> alignment fault if the SMBIOS table is of odd size. The fix is to
>>>
>>> replace memcpy with byte copy.
>>>
>>>
>>>
>>> Change-Id: I52a5be2fedcd057fdd5e510ff090e4129f128221
>>>
>>> ---
>>>
>>> src/dmi/dmicheck/dmicheck.c | 7 +++++--
>>>
>>> 1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>>
>>>
>>> diff --git a/src/dmi/dmicheck/dmicheck.c
>>> b/src/dmi/dmicheck/dmicheck.c
>>>
>>> index 1934ebe..98db76b 100644
>>>
>>> --- a/src/dmi/dmicheck/dmicheck.c
>>>
>>> +++ b/src/dmi/dmicheck/dmicheck.c
>>>
>>> @@ -370,8 +370,11 @@ static void* dmi_table_smbios30(fwts_framework
>>> *fw, fwts_smbios30_entry *entry)
>>>
>>> mem = fwts_mmap(addr, length);
>>>
>>> if (mem != FWTS_MAP_FAILED) {
>>>
>>> table = malloc(length);
>>>
>>> - if (table)
>>>
>>> - memcpy(table, mem, length);
>>>
>>> + if (table) {
>>>
>>> + size_t i = length;
>>>
>>> + while (i--)
>>>
>>> + ((uint8_t*)table)[i] =
>>> + ((uint8_t*)mem)[i];
>>>
>>> + }
>>>
>>> (void)fwts_munmap(mem, length);
>>>
>>> return table;
>>>
>>> }
>>>
>>> --
>>>
>>> 1.8.2.1
>>>
>> Is this a hypothetical issue or does it fix an observed issue with
>> non-aligned SMBIOS tables?
>>
>> Colin
>>
>>
> OK. I'm surprised that memcpy is tripping this, I thought it did unaligned
> copying correctly.
>
> I'm inclined to suggest adding a fwts_memcpy_unaligned() to
> src/lib/src/fwts_stringextras.c and the prototype in
> src/lib/include/fwts_stringextras.h and using this. I suspect we may need
> this kind of unaligned memcpy again in the future.
>
> Colin
>
Thanks!
Acked-by: Colin Ian King <colin.king at canonical.com>
More information about the fwts-devel
mailing list