1. Patchew
  2. Xen
  3. View series

[PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()

Andrew Cooper posted 1 patch 2 weeks, 1 day ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20240416155251.2942504-1-andrew.cooper3@citrix.com
xen/common/efi/pe.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
[PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()
Posted by Andrew Cooper 2 weeks, 1 day ago 
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(-)

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
Re: [PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()
Posted by Jan Beulich 1 week, 6 days ago 
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
Re: [PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()
Posted by Andrew Cooper 1 week, 6 days ago 
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
Re: [PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()
Posted by Roger Pau Monné 2 weeks ago 
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.
Re: [PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()
Posted by Andrew Cooper 1 week, 6 days ago 
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.
Re: [PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()
Posted by Jan Beulich 1 week, 6 days ago 
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
Re: [PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()
Posted by Andrew Cooper 1 week, 6 days ago 
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
Re: [PATCH] xen/efi: Rewrite DOS/PE magic checking without memcmp()
Posted by Stefano Stabellini 2 weeks, 1 day ago 
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
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.