Re: [RESEND V2] ieee1275/ofdisk: vscsi lun handling on lun len

[Daniel Kiper]

On Mon, Jun 10, 2024 at 11:29:56AM +0530, Mukesh Kumar Chaurasiya wrote:
> The information about "vscsi-report-luns" data is a list of disk details
> with pairs of memory addresses and lengths.
>
>                   8 bytes     8 bytes
> lun-addr  --->   ------------------------              8 bytes
>         ^        |  buf-addr | lun-count| --------> -------------
>         |        ------------------------           |   lun     |
>         |        |  buf-addr | lun-count| ----|     -------------
>      "len"       ------------------------     |     |  ...      |
>         |        |    ...               |     |     -------------
>         |        ------------------------     |     |   lun     |
>         |        |  buf-addr | lun-count|     |     -------------
>         V        ------------------------     |
>                                               |---> -------------
>                                                     |   lun     |
>                                                     -------------
>                                                     |  ...      |
>                                                     -------------
>                                                     |   lun     |
>                                                     -------------
> The way the expression (args.table + 4 + 8 * i) is used is incorrect and
> can be confusing. The list of LUNs doesn't end with NULL, indicated by
> while (*ptr). Usually, this loop doesn't process any LUNs because it ends
> before checking any as first reported LUN is likely to be 0. The list of
> LUNs ends based on its length, not by a NULL value.
>
> Signed-off-by: Mukesh Kumar Chaurasiya <mchauras@linux.ibm.com>
> ---
>  grub-core/disk/ieee1275/ofdisk.c | 27 ++++++++++++++++-----------
>  1 file changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/disk/ieee1275/ofdisk.c 
> b/grub-core/disk/ieee1275/ofdisk.c
> index c6cba0c8a..1618544a8 100644
> --- a/grub-core/disk/ieee1275/ofdisk.c
> +++ b/grub-core/disk/ieee1275/ofdisk.c
> @@ -222,8 +222,12 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>       grub_ieee1275_cell_t table;
>        }
>        args;
> +      struct lun_buf {
> +        grub_uint64_t *buf_addr;

Again, this is wrong taking into account diagram above. I think it
should be "grub_uint64_t buf_addr". Then you should convert it to the
pointer. Though please be cautious on the 32-bit platforms...

> +        grub_uint64_t lun_count;
> +      } *tbl;
>        char *buf, *bufptr;
> -      unsigned i;
> +      unsigned int i, j;
>
>        if (grub_ieee1275_open (alias->path, &ihandle))
>       return;
> @@ -248,17 +252,18 @@ dev_iterate (const struct grub_ieee1275_devalias *alias)
>       return;
>        bufptr = grub_stpcpy (buf, alias->path);
>
> +      tbl = (struct lun_len *) args.table;
>        for (i = 0; i < args.nentries; i++)
> -     {
> -       grub_uint64_t *ptr;
> -
> -       ptr = *(grub_uint64_t **) (args.table + 4 + 8 * i);
> -       while (*ptr)
> -         {
> -           grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, *ptr++);
> -           dev_iterate_real (buf, buf);
> -         }
> -     }
> +        {
> +          grub_uint64_t *ptr;
> +
> +          ptr = (grub_uint64_t *)(grub_addr_t) tbl[i].buf_addr;
> +          for (j = 0; j < tbl[i].lun_count; j++)
> +           {
> +             grub_snprintf (bufptr, 32, "/disk@%" PRIxGRUB_UINT64_T, *ptr++);
> +             dev_iterate_real (buf, buf);
> +           }
> +        }
>        grub_ieee1275_close (ihandle);
>        grub_free (buf);
>        return;

Daniel