| Message ID | 000101d24a80$6c139e20$443ada60$@codeaurora.org |
|---|---|
| State | Accepted |
| Headers | show |
On 29/11/16 20:37, wufan wrote: > From f507dd17853f7e17cecfa2d32d28448913c60873 Mon Sep 17 00:00:00 2001 > From: Fan Wu <wufan@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@canonical.com] > Sent: Monday, November 28, 2016 1:50 PM > To: wufan <wufan@codeaurora.org>; fwts-devel@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@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@canonical.com] >> Sent: Monday, November 28, 2016 1:08 PM >> To: wufan <wufan@codeaurora.org>; fwts-devel@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@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@canonical.com>
On 2016-11-29 12:37 PM, wufan wrote: > From f507dd17853f7e17cecfa2d32d28448913c60873 Mon Sep 17 00:00:00 2001 > From: Fan Wu <wufan@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@canonical.com] > Sent: Monday, November 28, 2016 1:50 PM > To: wufan <wufan@codeaurora.org>; fwts-devel@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@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@canonical.com] >> Sent: Monday, November 28, 2016 1:08 PM >> To: wufan <wufan@codeaurora.org>; fwts-devel@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@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 > > Acked-by: Alex Hung <alex.hung@canonical.com>
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];