Re: [PATCH v6 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.09.2020 13:51, Trammell Hudson wrote:
> @@ -624,6 +626,22 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, 
> CHAR16 *name,
>      return true;
>  }
>  
> +static bool __init read_section(const EFI_LOADED_IMAGE *image,
> +                                CHAR16 *name, struct file *file,
> +                                const char *options)
> +{
> +    file->ptr = pe_find_section(image->ImageBase, image->ImageSize,
> +                                name, &file->size);
> +    if ( !file->ptr )
> +        return false;
> +
> +    file->need_to_free = false;

In patch 2 you don't bother clearing the field, presumably because
it's static data and hence zero-filled anyway. This same assumption
would then hold here. Or else, to be consistent, the earlier patch
would want clearing added.

> +static int __init pe_name_compare(const struct PeSectionHeader *sect,
> +                                  const CHAR16 *name)
> +{
> +    size_t i;
> +
> +    if ( sect->Name[0] != '.' )
> +        return -1;
> +
> +    for ( i = 1; i < sizeof(sect->Name); i++ )
> +    {
> +        const char c = sect->Name[i];
> +        const CHAR16 cw = name[i-1];
> +        if ( cw == 0 && c == 0 )

Blank line between declarations and statements please.

> +            return 0;
> +        if ( cw < c )
> +            return -1;
> +        if ( cw > c )
> +            return +1;
> +     }
> +
> +     if ( name[i-1] < 0 )
> +         return -1;

I'm afraid this is liable to trigger compiler warnings, for checking
an unsigned quantity to be negative.

Also throughout here "i-1" wants spelling "i - 1".

> +const void *__init pe_find_section(const void *image, const UINTN image_size,
> +                                   const CHAR16 *section_name, UINTN 
> *size_out)
> +{
> +    const struct DosFileHeader *dos = image;
> +    const struct PeHeader *pe;
> +    const struct PeSectionHeader *sect;
> +    UINTN offset, i;
> +
> +    if ( image_size < sizeof(*dos) ||
> +         memcmp(dos->Magic, "MZ", 2) != 0 )
> +        return NULL;
> +
> +    offset = dos->ExeHeader;
> +    pe = image + offset;
> +
> +    offset += sizeof(*pe);
> +    if ( image_size < offset ||
> +         memcmp(pe->Magic, "PE\0\0", 4) != 0 )
> +        return NULL;
> +
> +    /* PE32+ Subsystem type */
> +    if ( pe->FileHeader.Machine != PE_HEADER_MACHINE )
> +        return NULL;

Comment and code don't look to be in line.

Jan