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

[Tobias Heider]

On Sat, Aug 10, 2024 at 3:38 AM Andrew Hamilton <adhamilt@gmail.com> wrote:
>
> Hello,
>
> I'm not an expert in GRUB, but was trying to learn more. I took this
> chance to learn about this new feature a bit and just had a couple
> thoughts / questions. Otherwise, thank you!
>
> > +char *
> > +grub_fdt_prop_to_string (const unsigned char *val, grub_uint32_t len)
> > +{
> > +  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)
> > +    {
> > +      /* byte string */
> > +      grub_printf ("[");
> > +
> > +      slen = (len * 3) + 2;
>
> Do you think a bounds check should be placed on 'len' before doing
> this calculation for 'slen' to ensure we do not exceed the storage
> capacity of an 'int' for 'slen'. Similar for later uses of 'pos'. Or
> perhaps, should a uint64_t be used for both 'slen'', 'pos', and 'i'?
>
> > +      /* cell list */
> > +      const grub_uint32_t *cell = (const grub_uint32_t *)val;
> > +
> > +      slen = (len * 11) + 2;
>
> Similar question here as above.

Thanks, I think you are right that it makes sense to consider overflows here.
In this specific case we are probably good without explicit bounds checks for
multiple reasons:

1. There is no criticial trust boundary involved. len here comes from
the device tree passed
    by the firmware. Unless the firmware is malicious (which would
come with other problems)
    it is probably safe to assume that those values stay within
reasonable limits.

2. In the more worst case `slen = (len * 11) + 2`, len would have to
be 195225786 or 195 MB
    to overflow slen. Now we are talking about device nodes, not
binary files so this seems way
    bigger than what we can reasonably expect. I was looking for an
official upper size limit in the
    device tree spec but found this instead:
        "The device tree blob (dtb) must be placed on an 8-byte
boundary and must
         not exceed 2 megabytes in size"
    from https://www.kernel.org/doc/Documentation/arm64/booting.txt.
    Note that this is the entire dtb, not a single property value.

So I think we are good unless my math is way off or I am missing something.

>
>  Thanks,
> Andrew
>
>
> On Thu, Aug 8, 2024 at 10:39 AM Tobias Heider
> <tobias.heider@canonical.com> 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
> > +prop_isstring (const unsigned char *str, grub_uint32_t len)
> > +{
> > +  grub_uint32_t i;
> > +  int is_nonzero = 0;
> > +
> > +  if (!len || str[len - 1])
> > +    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)
> > +{
> > +  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)
> > +    {
> > +      /* byte string */
> > +      grub_printf ("[");
> > +
> > +      slen = (len * 3) + 2;
> > +      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;
> > +
> > +      slen = (len * 11) + 2;
> > +      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;
> > +  char *str;
> >    void *fw_fdt;
> > +  grub_uint32_t len;
> >
> >    fw_fdt = grub_efi_get_firmware_fdt ();
> >    if (fw_fdt == NULL)
> > @@ -192,17 +194,22 @@ grub_cmd_fdtdump (grub_extcmd_context_t ctxt,
> >                           N_("No device tree found"));
> >
> >    if (state[0].set)
> > -      value = grub_fdt_get_prop (fw_fdt, 0, state[0].arg, NULL);
> > +      value = grub_fdt_get_prop (fw_fdt, 0, state[0].arg, &len);
> >
> >    if (value == NULL)
> >      return grub_error (GRUB_ERR_OUT_OF_RANGE,
> >                         N_("failed to retrieve the prop field"));
> >
> > +  str = grub_fdt_prop_to_string (value, len);
> > +  if (str == NULL)
> > +    return grub_error (GRUB_ERR_IO, N_("Failed to print string"));
> > +
> >    if (state[1].set)
> > -    grub_env_set (state[1].arg, value);
> > +    grub_env_set (state[1].arg, str);
> >    else
> > -    grub_printf ("%s\n", value);
> > +    grub_printf ("%s", str);
> >
> > +  grub_free (str);
> >    return GRUB_ERR_NONE;
> >  }
> >
> > diff --git a/include/grub/fdt.h b/include/grub/fdt.h
> > index e609c7e41..4db644544 100644
> > --- a/include/grub/fdt.h
> > +++ b/include/grub/fdt.h
> > @@ -118,6 +118,9 @@ EXPORT_FUNC(grub_fdt_get_nodename) (const void *fdt, 
> > unsigned int nodeoffset);
> >  const void *EXPORT_FUNC(grub_fdt_get_prop) (const void *fdt, unsigned int 
> > nodeoffset, const char *name,
> >                                             grub_uint32_t *len);
> >
> > +char *EXPORT_FUNC(grub_fdt_prop_to_string) (const unsigned char *val,
> > +                                            grub_uint32_t len);
> > +
> >  int EXPORT_FUNC(grub_fdt_set_prop) (void *fdt, unsigned int nodeoffset, 
> > const char *name,
> >                                     const void *val, grub_uint32_t len);
> >  #define grub_fdt_set_prop32(fdt, nodeoffset, name, val)        \
> > --
> > 2.43.0
> >
> >
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel