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

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

[Andrew Cooper]

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()

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

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
> 

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

[Roger Pau Monné]

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()

[Jan Beulich]

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()

[Jan Beulich]

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()

[Andrew Cooper]

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()

[Andrew Cooper]

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()

[Andrew Cooper]

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.