Re: [PATCH v2 03/11] x86/mkreloc: use the string table to get names

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.04.2025 15:08, Roger Pau Monne wrote:
> --- a/xen/arch/x86/efi/mkreloc.c
> +++ b/xen/arch/x86/efi/mkreloc.c
> @@ -17,6 +17,12 @@
>  #define PE_BASE_RELOC_HIGHLOW  3
>  #define PE_BASE_RELOC_DIR64   10
>  
> +/* The size of a symbol table entry is always 18 bytes. */
> +#define SYM_SIZE 18
> +
> +const char *string_table;
> +unsigned int string_table_size;

The way you use it, imo this wants to be uint32_t.

Both probably want to be static.

> @@ -25,6 +31,28 @@ static void usage(const char *cmd, int rc)
>      exit(rc);
>  }
>  
> +const char *get_name(const char *name)
> +{
> +    static char buffer[sizeof(((struct section_header *)NULL)->name) + 1] = 
> {};

As this makes things section specific, may I suggest to name the function
e.g. get_section_name()? Also better to be static again.

> +    unsigned long offset;
> +
> +    if ( name[0] != '/' )
> +    {
> +        /*
> +         * Use a temporary buffer in case the name is 8 characters long, as
> +         * then there's no terminating null character in the input string.
> +         */
> +        strncpy(buffer, name, sizeof(buffer) - 1);
> +        return buffer;
> +    }
> +
> +    offset = strtoul(&name[1], NULL, 10);

Don't you need to nul-terminate the string here, too, to play safe? (Yes,
we don't expect this big a string table.)

> +    if ( !string_table || offset < 4 || offset >= string_table_size )

Considering how you reduce string_table_size after having read it from
the image, don't you mean "offset - 4 >= string_table_size" here? Also
below you use sizeof(string_table_size) instead of a literal 4.

> +        return name;
> +
> +    return &string_table[offset - 4];

Here as well.

> @@ -83,6 +111,31 @@ static unsigned int load(const char *name, int *handle,
>          exit(3);
>      }
>  
> +    if ( !string_table && pe_hdr.symbol_table )
> +    {
> +        char *strings;
> +
> +        if ( lseek(in, pe_hdr.symbol_table + pe_hdr.symbols * SYM_SIZE,
> +                   SEEK_SET) < 0 ||
> +             read(in, &string_table_size, sizeof(string_table_size)) !=
> +             sizeof(string_table_size) )
> +        {
> +            perror(name);
> +            exit(3);
> +        }
> +
> +        string_table_size -= sizeof(string_table_size);

You're careful of underflow above; better be careful here, too?

> +        strings = malloc(string_table_size);

You check for all other errors, just not here?

Jan