Re: [PATCH] ofdisk: Recognizes SAS disks automatically

[Andrei Borzenkov <arvidjaar@gmail.com>]

В Fri, 31 Jul 2015 10:34:25 -0300
Paulo Flabiano Smorigo <pfsmorigo@linux.vnet.ibm.com> пишет:

> Read all children from sas path and add it to the device list.

Examples of paths and names would be useful for reference.

> 
> ---
>  grub-core/disk/ieee1275/ofdisk.c | 51 ++++++++++++++++++++++++++++++++++++++++
>  include/grub/ieee1275/ofdisk.h   |  3 +++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/grub-core/disk/ieee1275/ofdisk.c b/grub-core/disk/ieee1275/ofdisk.c
> index 331769b..b17f6d1 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -260,6 +260,57 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>        grub_free (buf);
>        return;
>      }
> +  else if (grub_strcmp (alias->type, "sas_ioa") == 0)
> +    {
> +      struct sas_children
> +        {
> +          struct grub_ieee1275_common_hdr common;
> +          grub_ieee1275_cell_t method;
> +          grub_ieee1275_cell_t ihandle;
> +          grub_ieee1275_cell_t max;
> +          grub_ieee1275_cell_t table;
> +          grub_ieee1275_cell_t catch_result;
> +          grub_ieee1275_cell_t nentries;
> +        }
> +      args;
> +      char *buf, *bufptr;
> +      unsigned i;
> +      grub_ieee1275_ihandle_t ihandle;
> +
> +      buf = grub_malloc (grub_strlen (alias->path)
> +                         + sizeof("/disk@") + IEEE1275_SAS_CHILDREN_SIZE + 1);
> +      if (!buf)
> +        return;
> +
> +      bufptr = grub_stpcpy (buf, alias->path);
> +
> +      if (grub_ieee1275_open (alias->path, &ihandle))
> +        return;

memory leak.

> +
> +      INIT_IEEE1275_COMMON (&args.common, "call-method", 4, 2);
> +      args.method = (grub_ieee1275_cell_t) "get-sas-children";
> +      args.ihandle = ihandle;
> +      args.max = IEEE1275_SAS_CHILDREN_BUFFER_SIZE;
> +      args.table = 0;
> +      args.catch_result = 0;
> +      args.nentries = 0;
> +
> +      if (IEEE1275_CALL_ENTRY_FN (&args) == -1)
> +        {
> +          grub_ieee1275_close (ihandle);
> +          return;

memory leak

> +        }
> +      grub_uint64_t *ptr;
> +      for (i = 0; i < args.nentries; i++)
> +        {
> +          ptr = (grub_uint64_t *) (args.table + IEEE1275_SAS_CHILDREN_SIZE * i);

Is it guaranteed to be aligned? Why do you need magic constant if it
really sizeof(grub_unit64_t)?

> +          grub_snprintf (bufptr, 100, "/disk@%" PRIxGRUB_UINT64_T, (grub_uint64_t) *ptr);

Where size 100 comes from? You just allocated 8 bytes for the rest
(sans final zero byte), but printf outputs 16 bytes, so it overflows
buffer.

> +          dev_iterate_real (buf, buf);
> +        }
> +
> +      grub_ieee1275_close (ihandle);

Out of curiosity - what happens with args.table? Does firmware free it
implicitly?

> +      grub_free (buf);
> +    }
>  
>    if (!grub_ieee1275_test_flag (GRUB_IEEE1275_FLAG_NO_TREE_SCANNING_FOR_DISKS)
>        && grub_strcmp (alias->type, "block") == 0)
> diff --git a/include/grub/ieee1275/ofdisk.h b/include/grub/ieee1275/ofdisk.h
> index 2f69e3f..59f84f7 100644
> --- a/include/grub/ieee1275/ofdisk.h
> +++ b/include/grub/ieee1275/ofdisk.h
> @@ -19,6 +19,9 @@
>  #ifndef GRUB_OFDISK_HEADER
>  #define GRUB_OFDISK_HEADER	1
>  
> +#define IEEE1275_SAS_CHILDREN_SIZE 8
> +#define IEEE1275_SAS_CHILDREN_BUFFER_SIZE 100*IEEE1275_SAS_CHILDREN_SIZE
> +
>  extern void grub_ofdisk_init (void);
>  extern void grub_ofdisk_fini (void);
>