Re: [PATCH 1/2] fdtdump: add grub_fdt_prop_to_string() for safe string conversion

[Daniel Kiper]

On Thu, Aug 08, 2024 at 05:37:46PM +0200, Tobias Heider wrote:
> Device tree properties are not explicitly typed but values can take
> multiple forms from strings to arrays and byte-strings.
> grub_fdt_prop_to_string() adds a heuristic to determine the type and
> convert it to a string for printing.
>
> Signed-off-by: Tobias Heider <tobias.heider@canonical.com>
> ---
>  grub-core/lib/fdt.c        | 87 ++++++++++++++++++++++++++++++++++++++
>  grub-core/loader/efi/fdt.c | 15 +++++--
>  include/grub/fdt.h         |  3 ++
>  3 files changed, 101 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/lib/fdt.c b/grub-core/lib/fdt.c
> index 73cfa94a2..5ec32ac56 100644
> --- a/grub-core/lib/fdt.c
> +++ b/grub-core/lib/fdt.c
> @@ -1,6 +1,7 @@
>  /*
>   *  GRUB  --  GRand Unified Bootloader
>   *  Copyright (C) 2013  Free Software Foundation, Inc.
> + *  Copyright (C) 2024  Canonical, Ltd.
>   *
>   *  GRUB is free software: you can redistribute it and/or modify
>   *  it under the terms of the GNU General Public License as published by
> @@ -501,6 +502,92 @@ int grub_fdt_set_prop (void *fdt, unsigned int 
> nodeoffset, const char *name,
>    return 0;
>  }
>
> +static int

s/int/bool/

> +prop_isstring (const unsigned char *str, grub_uint32_t len)

I think you can drop unsigned here.

> +{
> +  grub_uint32_t i;
> +  int is_nonzero = 0;

Please use bool instead of int.

> +  if (!len || str[len - 1])

If you are doing "str[len - 1]" then I think you should put a comment
before function saying that the caller should check len.

> +    return 0;
> +
> +  for (i = 0; i < len; i++)
> +    {
> +      if (!str[i])
> +        {
> +          if (!is_nonzero)
> +            return 0;
> +
> +          is_nonzero = 0;
> +          continue;
> +        }
> +      if (!grub_isprint (str[i]))
> +        return 0;
> +
> +      is_nonzero = 1;
> +    }
> +  return 1;
> +}
> +
> +char *
> +grub_fdt_prop_to_string (const unsigned char *val, grub_uint32_t len)

Again, I think you can drop unsigned here.

> +{
> +  char *str;
> +  int pos, slen;
> +  grub_uint32_t i;
> +
> +  if (prop_isstring (val, len))
> +    {
> +      /* string */
> +      return grub_strdup ((const char *)val);
> +    }
> +  else if (len & 0x3)

Could you define 0x3 as a constant? Or at least add a comment what it means.

> +    {
> +      /* byte string */
> +      grub_printf ("[");

This grub_printf() looks strange. What it does...

> +      slen = (len * 3) + 2;

Does len come from trusted source? If yes it requires a comment here.
And I think slen should be defined as grub_size_t.

> +      str = grub_malloc (slen);
> +      if (str == NULL)
> +        return NULL;
> +
> +      pos = 0;
> +      str[pos++] = '[';
> +      for (i = 0; i < len; i++)
> +        {
> +          pos += grub_snprintf (&str[pos], slen - pos, "%02x", val[i]);
> +          if (i != len - 1)
> +            str[pos++] = ' ';
> +        }
> +      str[pos++] = ']';
> +      str[pos] = '\0';
> +    }
> +  else
> +    {
> +      /* cell list */
> +      const grub_uint32_t *cell = (const grub_uint32_t *)val;

Please add space after ")".

> +      slen = (len * 11) + 2;

Where do these numbers come from? Please add a comment here and/or
define constants. Similarly it should be explained above.

> +      str = grub_malloc (slen);
> +      if (str == NULL)
> +        return NULL;
> +
> +      pos = 0;
> +      str[pos++] = '<';
> +      for (i = 0, len /= 4; i < len; i++)
> +        {
> +          pos += grub_snprintf (&str[pos], slen - pos, "0x%08x",
> +                                grub_be_to_cpu32 (cell[i]));
> +          if (i != len - 1)
> +            str[pos++] = ' ';
> +        }
> +      str[pos++] = '>';
> +      str[pos] = '\0';
> +    }
> +
> +  return str;
> +}
> +
>  int
>  grub_fdt_create_empty_tree (void *fdt, unsigned int size)
>  {
> diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> index e510b3491..b43ff730d 100644
> --- a/grub-core/loader/efi/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -183,8 +183,10 @@ grub_cmd_fdtdump (grub_extcmd_context_t ctxt,
>                   char **argv __attribute__ ((unused)))
>  {
>    struct grub_arg_list *state = ctxt->state;
> -  const char *value = NULL;
> +  const unsigned char *value = NULL;

Why do you add unsigned here? If it is really needed it should be
explained in the commit message.

Daniel