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,
Andrew
On Thu, Aug 8, 2024 at 10:39 AM Tobias Heider
<tobias.hei...@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.hei...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
