| Message ID | 20240416155251.2942504-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/efi: Rewrite DOS/PE magic checking without memcmp() | expand |
On Tue, 16 Apr 2024, Andrew Cooper wrote: > Misra Rule 21.16 doesn't like the use of memcmp() between a string literal and > a UINT8 array. Rewrite using plain compares. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: consulting@bugseng.com <consulting@bugseng.com> > CC: Roberto Bagnara <roberto.bagnara@bugseng.com> > CC: Federico Serafini <federico.serafini@bugseng.com> > CC: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/common/efi/pe.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c > index a84992df9afe..ef8a2543e0a1 100644 > --- a/xen/common/efi/pe.c > +++ b/xen/common/efi/pe.c > @@ -111,7 +111,8 @@ const void *__init pe_find_section(const void *image, const UINTN image_size, > UINTN offset, i; > > if ( image_size < sizeof(*dos) || > - memcmp(dos->Magic, "MZ", 2) != 0 ) > + dos->Magic[0] != 'M' || > + dos->Magic[1] != 'Z' ) > return NULL; > > offset = dos->ExeHeader; > @@ -119,7 +120,10 @@ const void *__init pe_find_section(const void *image, const UINTN image_size, > > offset += sizeof(*pe); > if ( image_size < offset || > - memcmp(pe->Magic, "PE\0\0", 4) != 0 ) > + pe->Magic[0] != 'P' || > + pe->Magic[1] != 'E' || > + pe->Magic[2] != '\0' || > + pe->Magic[3] != '\0' ) > return NULL; > > if ( pe->FileHeader.Machine != PE_HEADER_MACHINE ) > > base-commit: c0f890cd9d5fd2c17a1e3110cb26f98c90ce8429 > -- > 2.30.2 >
On Tue, Apr 16, 2024 at 04:52:51PM +0100, Andrew Cooper wrote: > Misra Rule 21.16 doesn't like the use of memcmp() between a string literal and > a UINT8 array. Rewrite using plain compares. The commit message makes it look like it's a type mismatch issue between the two elements being compared, but from my reading of the rule the issue is with the usage of a char pointer with memcmp(). IOW: even if the two parameters are char pointers it would still be a violation. "Misra Rule 21.16 forbids the use of memcmp() against character arrays. Rewrite using plain compares since checking for "PE\0\0" cannot be done using strncmp()." > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> LGTM (possibly pending the adjustment of the commit message): Acked-by: Roger Pau Monné <roger.pau@citrix.com> One question below to ensure my understating is correct. > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: consulting@bugseng.com <consulting@bugseng.com> > CC: Roberto Bagnara <roberto.bagnara@bugseng.com> > CC: Federico Serafini <federico.serafini@bugseng.com> > CC: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > xen/common/efi/pe.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c > index a84992df9afe..ef8a2543e0a1 100644 > --- a/xen/common/efi/pe.c > +++ b/xen/common/efi/pe.c > @@ -111,7 +111,8 @@ const void *__init pe_find_section(const void *image, const UINTN image_size, > UINTN offset, i; > > if ( image_size < sizeof(*dos) || > - memcmp(dos->Magic, "MZ", 2) != 0 ) > + dos->Magic[0] != 'M' || > + dos->Magic[1] != 'Z' ) For this one you could likely use strncmp()? > return NULL; > > offset = dos->ExeHeader; > @@ -119,7 +120,10 @@ const void *__init pe_find_section(const void *image, const UINTN image_size, > > offset += sizeof(*pe); > if ( image_size < offset || > - memcmp(pe->Magic, "PE\0\0", 4) != 0 ) > + pe->Magic[0] != 'P' || > + pe->Magic[1] != 'E' || > + pe->Magic[2] != '\0' || > + pe->Magic[3] != '\0' ) This one with the double null terminator is indeed not suitable to be checked using strncmp(). Thanks, Roger.
On 17.04.2024 09:14, Roger Pau Monné wrote: > On Tue, Apr 16, 2024 at 04:52:51PM +0100, Andrew Cooper wrote: >> --- a/xen/common/efi/pe.c >> +++ b/xen/common/efi/pe.c >> @@ -111,7 +111,8 @@ const void *__init pe_find_section(const void *image, const UINTN image_size, >> UINTN offset, i; >> >> if ( image_size < sizeof(*dos) || >> - memcmp(dos->Magic, "MZ", 2) != 0 ) >> + dos->Magic[0] != 'M' || >> + dos->Magic[1] != 'Z' ) > > For this one you could likely use strncmp()? strncmp() against UINT8[2] wouldn't be liked by the compiler, I guess. Jan
On 16.04.2024 17:52, Andrew Cooper wrote: > Misra Rule 21.16 doesn't like the use of memcmp() between a string literal and > a UINT8 array. Rewrite using plain compares. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> after having realized that sadly gcc13 instantiates the compound literals (that I had thought of using) in .rodata (or one of its variants), rather than leveraging them being as constant as string literals. Jan
On 18/04/2024 12:06 pm, Jan Beulich wrote: > On 17.04.2024 09:14, Roger Pau Monné wrote: >> On Tue, Apr 16, 2024 at 04:52:51PM +0100, Andrew Cooper wrote: >>> --- a/xen/common/efi/pe.c >>> +++ b/xen/common/efi/pe.c >>> @@ -111,7 +111,8 @@ const void *__init pe_find_section(const void *image, const UINTN image_size, >>> UINTN offset, i; >>> >>> if ( image_size < sizeof(*dos) || >>> - memcmp(dos->Magic, "MZ", 2) != 0 ) >>> + dos->Magic[0] != 'M' || >>> + dos->Magic[1] != 'Z' ) >> For this one you could likely use strncmp()? > strncmp() against UINT8[2] wouldn't be liked by the compiler, I guess. Indeed. And this MISRA rule is very much "you are mixing string and non-string types. Don't do that." This is a very rare patten, where we are looking for a binary marker than just happens to also make sense when expressed as an ASCII string. Although the MISRA complaint does raise a good point. The memcmp() form would malfunction on any system with CHAR_BIT != 8, in a way that the {u,}int8_t-at-a-time form wouldn't. ~Andrew
On 18/04/2024 12:09 pm, Jan Beulich wrote: > On 16.04.2024 17:52, Andrew Cooper wrote: >> Misra Rule 21.16 doesn't like the use of memcmp() between a string literal and >> a UINT8 array. Rewrite using plain compares. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. > after having realized that sadly gcc13 instantiates the compound literals > (that I had thought of using) in .rodata (or one of its variants), rather > than leveraging them being as constant as string literals. gcc10 manages to optimise both checks to a cmp{w,l}, which I did check before sending the patch. However, it chooses a different base register for the cmpl and there's a sad cascade effect where a bunch of JMP disp8's turn into disp32's. But oh well - it's init code. ~Andrew
On 17/04/2024 8:14 am, Roger Pau Monné wrote: > On Tue, Apr 16, 2024 at 04:52:51PM +0100, Andrew Cooper wrote: >> Misra Rule 21.16 doesn't like the use of memcmp() between a string literal and >> a UINT8 array. Rewrite using plain compares. > The commit message makes it look like it's a type mismatch issue > between the two elements being compared, but from my reading of the > rule the issue is with the usage of a char pointer with memcmp(). > IOW: even if the two parameters are char pointers it would still be a > violation. > > "Misra Rule 21.16 forbids the use of memcmp() against character > arrays. Rewrite using plain compares since checking for "PE\0\0" > cannot be done using strncmp()." I've tweaked the sentence to say character array, but IMO it's really not about strncmp() which wouldn't be correct to use here even if it happened to produce a correct answer. >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > LGTM (possibly pending the adjustment of the commit message): > > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks.
diff --git a/xen/common/efi/pe.c b/xen/common/efi/pe.c index a84992df9afe..ef8a2543e0a1 100644 --- a/xen/common/efi/pe.c +++ b/xen/common/efi/pe.c @@ -111,7 +111,8 @@ const void *__init pe_find_section(const void *image, const UINTN image_size, UINTN offset, i; if ( image_size < sizeof(*dos) || - memcmp(dos->Magic, "MZ", 2) != 0 ) + dos->Magic[0] != 'M' || + dos->Magic[1] != 'Z' ) return NULL; offset = dos->ExeHeader; @@ -119,7 +120,10 @@ const void *__init pe_find_section(const void *image, const UINTN image_size, offset += sizeof(*pe); if ( image_size < offset || - memcmp(pe->Magic, "PE\0\0", 4) != 0 ) + pe->Magic[0] != 'P' || + pe->Magic[1] != 'E' || + pe->Magic[2] != '\0' || + pe->Magic[3] != '\0' ) return NULL; if ( pe->FileHeader.Machine != PE_HEADER_MACHINE )
Misra Rule 21.16 doesn't like the use of memcmp() between a string literal and a UINT8 array. Rewrite using plain compares. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: consulting@bugseng.com <consulting@bugseng.com> CC: Roberto Bagnara <roberto.bagnara@bugseng.com> CC: Federico Serafini <federico.serafini@bugseng.com> CC: Nicola Vetrini <nicola.vetrini@bugseng.com> --- xen/common/efi/pe.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) base-commit: c0f890cd9d5fd2c17a1e3110cb26f98c90ce8429