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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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@xxxxxxxxxx>

LGTM (possibly pending the adjustment of the commit message):

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

One question below to ensure my understating is correct.

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: consulting@xxxxxxxxxxx <consulting@xxxxxxxxxxx>
> CC: Roberto Bagnara <roberto.bagnara@xxxxxxxxxxx>
> CC: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> CC: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
> ---
>  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.