[PATCH] fwts:dmicheck: replace memcpy with byte copy for SMBIOS 3.0 table

[Colin Ian King]

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