Re: [PATCH v4 5/5] ieee1275: ofpath enable NVMeoF logical device translate

[Avnish Chouhan <avnish@linux.ibm.com>]

Hi Michael,
Thank you so much for a review!

I'll add the failure checks in the code mentioned by you.

On build issue. I haven't observed any of these errors on my machine. 
Today, I built the latest upstream grub with this patch on my machine 
and no errors are observed.

Regards,
Avnish Chouhan

On 2025-02-12 13:44, Michael Chang wrote:
> Hi Avnish,
> 
> This patch doesn't build:
> 
>   ../grub-core/osdep/linux/ofpath.c: In function ‘of_path_of_nvme’:
>   ../grub-core/osdep/linux/ofpath.c:729:37: error: ‘%x’ directive
> output may be truncated writing between 1 and 8 bytes into a region of
> size 7 [-Werror=format-truncation=]
>     729 |                         "/namespace@%x:%d", nsid, part);
> 	|                                     ^~
>   ../grub-core/osdep/linux/ofpath.c:729:25: note: directive argument
> in the range [1, 4294967295]
>     729 |                         "/namespace@%x:%d", nsid, part);
> 	|                         ^~~~~~~~~~~~~~~~~~
>   ../grub-core/osdep/linux/ofpath.c:728:15: note: ‘snprintf’ output
> between 15 and 32 bytes into a destination of size 18
>     728 |               snprintf (disk+chars_written,
> sizeof("/namespace@") + MAX_NVME_NSID_DIGITS,
> 	|
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     729 |                         "/namespace@%x:%d", nsid, part);
> 	|                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   ../grub-core/osdep/linux/ofpath.c:770:37: error: ‘%x’ directive
> output may be truncated writing between 1 and 8 bytes into a region of
> size 7 [-Werror=format-truncation=]
>     770 |                         "/namespace@%x", nsid);
> 	|                                     ^~
>   ../grub-core/osdep/linux/ofpath.c:770:25: note: directive argument
> in the range [1, 4294967295]
>     770 |                         "/namespace@%x", nsid);
> 	|                         ^~~~~~~~~~~~~~~
>   ../grub-core/osdep/linux/ofpath.c:769:15: note: ‘snprintf’ output
> between 13 and 20 bytes into a destination of size 18
>     769 |               snprintf
> (disk+chars_written,sizeof("/namespace@") + sizeof(char) *
> MAX_NVME_NSID_DIGITS,
> 	|
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>     770 |                         "/namespace@%x", nsid);
> 	|                         ~~~~~~~~~~~~~~~~~~~~~~
>   cc1: all warnings being treated as errors
> 
> The error is valid. It is pointing to that the size,
> MAX_NVME_NSID_DIGITS, provided for snprintf() is too small to the worse
> case of format specifier output, thus the result truncated.
> 
> The `gcc --version`:
>   gcc (SUSE Linux) 14.2.1 20241007 [revision
> 4af44f2cf7d281f3e4f3957efce10e8b2ccb2ad3]
> 
> Moreover, it has a few critical memory related issue that may crash the
> program. (See below)
> 
> On Fri, Aug 30, 2024 at 05:11:04PM +0530, Avnish Chouhan wrote:
>> This patch adds code to enable the translation of logical devices to 
>> the of NVMeoFC paths.
>> 
>> Signed-off-by: Diego Domingos <diegodo@br.ibm.com>
>> Signed-off-by: Avnish Chouhan <avnish@linux.ibm.com>
>> ---
>>  grub-core/osdep/linux/ofpath.c | 371 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  include/grub/util/ofpath.h     | 28 ++++++++++++++++++++++++++++
>>  2 files changed, 390 insertions(+), 9 deletions(-)
>> 
>> diff --git a/grub-core/osdep/linux/ofpath.c 
>> b/grub-core/osdep/linux/ofpath.c
>> index 0f5d54e..f0e7dcc 100644
>> --- a/grub-core/osdep/linux/ofpath.c
>> +++ b/grub-core/osdep/linux/ofpath.c
>> @@ -37,6 +37,7 @@
>>  #include <fcntl.h>
>>  #include <errno.h>
>>  #include <ctype.h>
>> +#include <dirent.h>
>> 
>>  #ifdef __sparc__
>>  typedef enum
>> @@ -136,7 +137,7 @@ trim_newline (char *path)
>>      *end-- = '\0';
>>  }
>> 
>> -#define MAX_DISK_CAT    64
>> +#define MAX_DISK_CAT    512
>> 
>>  static char *
>>  find_obppath (const char *sysfs_path_orig)
>> @@ -312,6 +313,91 @@ get_basename(char *p)
>>    return ret;
>>  }
>> 
>> +int
>> +add_filename_to_pile (char *filename, struct ofpath_files_list_root* 
>> root)
>> +{
>> +  struct ofpath_files_list_node* file;
>> +
>> +  file = malloc (sizeof (struct ofpath_files_list_node));
>> +  if (!file)
>> +    return -1;
>> +
>> +  file->filename = malloc (sizeof (char) * 1024);
>> +  if (!file->filename)
>> +    {
>> +      free (file);
>> +      return -1;
>> +    }
>> +
>> +  grub_strcpy (file->filename, filename);
>> +  if (root->first == NULL)
>> +    {
>> +      root->items = 1;
>> +      root->first = file;
>> +      file->next = NULL;
>> +    }
>> +  else
>> +    {
>> +      root->items++;
>> +      file->next = root->first;
>> +      root->first = file;
>> +    }
>> +
>> +  return 0;
>> +}
>> +
>> +void
>> +find_file (char* filename, char* directory, struct 
>> ofpath_files_list_root* root, int max_depth, int depth)
>> +{
>> +  struct dirent *ep;
>> +  struct stat statbuf;
>> +  DIR *dp;
>> +  int ret_val=0;
>> +  char* full_path;
>> +
>> +  if (depth > max_depth)
>> +    {
>> +      return;
>> +    }
>> +
>> +  if ((dp = opendir (directory)) == NULL)
>> +    {
>> +      return;
>> +    }
>> +
>> +  full_path = malloc (1024 * sizeof (char));
>> +  if (!full_path)
>> +    return;
>> +
>> +  while ((ep = readdir(dp)) != NULL)
>> +    {
>> +      snprintf (full_path, 1024, "%s/%s", directory, ep->d_name);
>> +      lstat (full_path, &statbuf);
>> +
>> +      if (S_ISLNK (statbuf.st_mode))
>> +        {
>> +          continue;
>> +        }
>> +
>> +      if (!strcmp (ep->d_name, ".") || !strcmp(ep->d_name, ".."))
>> +        {
>> +          continue;
>> +        }
>> +
>> +      if (!strcmp (ep->d_name, filename))
>> +        {
>> +          ret_val = add_filename_to_pile (full_path, root);
>> +          if (ret_val == -1)
>> +            continue;
>> +        }
>> +
>> +      find_file (filename, full_path, root, max_depth, depth+1);
>> +    }
>> +
>> +  free (full_path);
>> +  closedir (dp);
>> +}
>> +
>>  static char *
>>  of_path_of_vdisk(const char *sys_devname __attribute__((unused)),
>>  		 const char *device,
>> @@ -382,7 +468,200 @@ of_fc_port_name (const char *path, const char 
>> *subpath, char *port_name)
>>    free (basepath);
>>  }
>> 
>> -#ifdef __sparc__
>> +void
>> +free_ofpath_files_list (struct ofpath_files_list_root* root)
>> +{
>> +  struct ofpath_files_list_node* node = root->first;
>> +  struct ofpath_files_list_node* next;
>> +
>> +  while (node!=NULL)
>> +    {
>> +      next = node->next;
>> +      free (node->filename);
>> +      free (node);
>> +      node = next;
>> +    }
>> +
>> +  free (root);
>> +  return;
>> +}
>> +
>> +char*
>> +of_find_fc_host (char* host_wwpn)
>> +{
>> +  FILE* fp;
>> +  char *buf;
>> +  char *ret_val;
>> +  char portname_filename[sizeof ("port_name")] = "port_name";
>> +  char devices_path[sizeof ("/sys/devices")] = "/sys/devices";
>> +  struct ofpath_files_list_root* portnames_file_list;
>> +  struct ofpath_files_list_node* node;
>> +
>> +  ret_val = malloc (sizeof (char) * 1024);
>> +  if (!ret_val)
>> +    return NULL;
>> +
>> +  portnames_file_list = malloc (sizeof (struct 
>> ofpath_files_list_root));
>> +  if (!portnames_file_list)
>> +    {
>> +      free (ret_val);
>> +      return NULL;
>> +    }
>> +
>> +  portnames_file_list->items = 0;
>> +  portnames_file_list->first = NULL;
>> +  find_file (portname_filename, devices_path, portnames_file_list, 
>> 10, 0);
>> +  node = portnames_file_list->first;
>> +
>> +  while (node != NULL)
>> +    {
>> +      fp = fopen(node->filename, "r");
>> +      buf = malloc (sizeof (char) * 512);
>> +      if (!buf)
>> +        break;
>> +
>> +      fscanf (fp, "%s", buf);
>> +      fclose (fp);
>> +
>> +      if ((strcmp (buf, host_wwpn) == 0) && grub_strstr 
>> (node->filename, "fc_host"))
>> +        {
>> +          free (buf);
>> +          grub_strcpy (ret_val, node->filename);
>> +          free_ofpath_files_list (portnames_file_list);
>> +          return ret_val;
>> +        }
>> +
>> +      node = node->next;
>> +      free (buf);
>> +    }
>> +  free_ofpath_files_list (portnames_file_list);
>> +  free (ret_val);
>> +  return NULL;
>> +}
>> +
>> +int
>> +of_path_get_nvmeof_adapter_info (char* sysfs_path,
>> +                                 struct ofpath_nvmeof_info* 
>> nvmeof_info)
>> +{
>> +  FILE *fp;
>> +  char *buf, *buf2, *buf3;
>> +
>> +  nvmeof_info->host_wwpn = malloc (sizeof (char) * 256);
>> +  nvmeof_info->target_wwpn = malloc (sizeof (char) * 256);
>> +  nvmeof_info->nqn = malloc (sizeof (char) * 256);
>> +
>> +  if (nvmeof_info->host_wwpn == NULL || nvmeof_info->target_wwpn == 
>> NULL || nvmeof_info->nqn == NULL)
>> +    {
>> +      free (nvmeof_info->host_wwpn);
>> +      free (nvmeof_info->target_wwpn);
>> +      free (nvmeof_info->nqn);
>> +      return -1;
>> +    }
>> +
>> +  buf = malloc (sizeof (char) * 512);
>> +  if (!buf)
>> +    {
>> +      free (nvmeof_info->host_wwpn);
>> +      free (nvmeof_info->target_wwpn);
>> +      free (nvmeof_info->nqn);
>> +      return -1;
>> +    }
>> +
>> +  snprintf (buf, 512, "%s/subsysnqn", sysfs_path);
>> +  fp = fopen (buf, "r");
> 
> The fp == NULL check is missing.
> 
>> +  fscanf (fp, "%s", nvmeof_info->nqn);
> 
> The return value of fscanf() should be checked to verify that the nqn
> read from the file is successful.
> 
>> +  fclose (fp);
>> +
>> +  snprintf (buf, 512, "%s/cntlid", sysfs_path);
>> +  fp = fopen (buf, "r");
> 
> Ditto.
> 
>> +  fscanf (fp, "%u", &(nvmeof_info->cntlid));
> 
> Ditto.
> 
>> +  fclose (fp);
>> +
>> +  snprintf (buf, 512, "%s/address", sysfs_path);
>> +  fp = fopen (buf, "r");
> 
> Ditto.
> 
>> +  buf2 = malloc (sizeof (char) * 512);
>> +
>> +  if (!buf2)
>> +    {
>> +      free (nvmeof_info->host_wwpn);
>> +      free (nvmeof_info->target_wwpn);
>> +      free (nvmeof_info->nqn);
>> +      free (buf);
>> +      return -1;
>> +    }
>> +
>> +  fscanf (fp, "%s", buf2);
> 
> Ditto.
> 
>> +  fclose (fp);
>> +
>> +  buf3 = strrchr (buf2, '-') + 1;
> 
> Missing check for strrchr() != NULL so that buf3 may be assigned 
> invalid
> address "1". This is very likely to happen given no check for buf2 read
> from file is successful.
> 
>> +  grub_memcpy (nvmeof_info->host_wwpn, buf3, 256);
> 
> The strrchr()/strchr() may return NULL if the characters being searched
> is not present. As a consequence this will cause segvfault by reading
> illegal address "1".
> 
>> +  buf3=strchr (buf2, '-') + 1;
>> +  buf3=strchr (buf3, '-') + 1;
>> +  buf3=strchr (buf3, 'x') + 1;
>> +  grub_memcpy (nvmeof_info->target_wwpn, buf3, 256);
> 
> Ditto.
> 
>> +  buf3 = strchr (nvmeof_info->target_wwpn, ',');
>> +  *buf3 = '\0';
> 
> The buf3 != NULL check is missing before dereference.
> 
>> +  free (buf);
>> +  free (buf2);
>> +  return 0;
>> +}
>> +
>> +#define MAX_NVME_NSID_DIGITS 6
>> +
>> +static char *
>> +of_path_get_nvme_controller_name_node (const char* devname)
>> +{
>> +  char *controller_node, *end;
>> +
>> +  controller_node = strdup (devname);
>> +  end = grub_strchr (controller_node + 1, 'n');
>> +  if (end != NULL)
>> +    {
>> +      *end = '\0';
>> +    }
>> +
>> +  return controller_node;
>> +}
>> +
>> +unsigned int
>> +of_path_get_nvme_nsid (const char* devname)
>> +{
>> +  unsigned int nsid;
>> +  char *sysfs_path, *buf;
>> +  FILE *fp;
>> +
>> +  buf = malloc (sizeof(char) * 512);
>> +  if (!buf)
>> +    return 0;
>> +
>> +  sysfs_path = block_device_get_sysfs_path_and_link (devname);
>> +  snprintf (buf, 512, "%s/%s/nsid", sysfs_path, devname);
>> +  fp = fopen(buf, "r");
>> +  fscanf (fp, "%u", &(nsid));
>> +  fclose (fp);
> 
> Ditto
> 
>> +
>> +  free (sysfs_path);
>> +  free (buf);
>> +  return nsid;
>> +}
>> +
>> +static char *
>> +nvme_get_syspath (const char *nvmedev)
>> +{
>> +  char *sysfs_path, *controller_node;
>> +
>> +  sysfs_path = block_device_get_sysfs_path_and_link (nvmedev);
>> +  if (strstr (sysfs_path, "nvme-subsystem"))
>> +    {
>> +      controller_node = of_path_get_nvme_controller_name_node 
>> (nvmedev);
>> +      strcat (sysfs_path, "/");
>> +      strcat (sysfs_path, controller_node);
> 
> The strcat() would lead to out-of-bounds writes as the destination
> buffer, sysfs_path, didn't have more space to cancat any new string.
> 
> Thanks,
> Michael
> 
>> +      sysfs_path = xrealpath (sysfs_path);
>> +    }
>> +
>> +  return sysfs_path;
>> +}
>> +
>>  static char *
>>  of_path_of_nvme(const char *sys_devname __attribute__((unused)),
>>  	        const char *device,
>> @@ -391,6 +670,8 @@ of_path_of_nvme(const char *sys_devname 
>> __attribute__((unused)),
>>  {
>>    char *sysfs_path, *of_path, disk[MAX_DISK_CAT];
>>    const char *digit_string, *part_end;
>> +  int chars_written, ret_val;
>> +  struct ofpath_nvmeof_info* nvmeof_info;
>> 
>>    digit_string = trailing_digits (device);
>>    part_end = devicenode + strlen (devicenode) - 1;
>> @@ -410,15 +691,90 @@ of_path_of_nvme(const char *sys_devname 
>> __attribute__((unused)),
>>        /* Remove the p. */
>>        *end = '\0';
>>        sscanf (digit_string, "%d", &part);
>> -      snprintf (disk, sizeof (disk), "/disk@1:%c", 'a' + (part - 1));
>> -      sysfs_path = block_device_get_sysfs_path_and_link (nvmedev);
>> +      sysfs_path = nvme_get_syspath (nvmedev);
>> +
>> +      /* If is a NVMeoF */
>> +      if (strstr (sysfs_path, "nvme-fabrics"))
>> +        {
>> +          nvmeof_info = malloc (sizeof (struct ofpath_nvmeof_info));
>> +          if (!nvmeof_info)
>> +            {
>> +              free (nvmedev);
>> +              return NULL;
>> +            }
>> +
>> +          ret_val = of_path_get_nvmeof_adapter_info (sysfs_path, 
>> nvmeof_info);
>> +          if (ret_val == -1)
>> +            {
>> +              free (nvmedev);
>> +              free (nvmeof_info);
>> +              return NULL;
>> +            }
>> +
>> +          sysfs_path = of_find_fc_host (nvmeof_info->host_wwpn);
>> +          if (!sysfs_path)
>> +            {
>> +              free (nvmedev);
>> +              free (nvmeof_info);
>> +              return NULL;
>> +            }
>> +
>> +          chars_written = snprintf (disk,sizeof(disk), 
>> "/nvme-of/controller@%s,%x:nqn=%s",
>> +                                    nvmeof_info->target_wwpn,0xffff,
>> +                                    nvmeof_info->nqn);
>> +          unsigned int nsid = of_path_get_nvme_nsid (nvmedev);
>> +          if (nsid)
>> +            {
>> +              snprintf (disk+chars_written, sizeof("/namespace@") + 
>> MAX_NVME_NSID_DIGITS,
>> +                        "/namespace@%x:%d", nsid, part);
>> +            }
>> +          free (nvmeof_info);
>> +        }
>> +      else
>> +        {
>> +          snprintf (disk, sizeof (disk), "/disk@1:%c", 'a' + (part - 
>> 1));
>> +        }
>>        free (nvmedev);
>>      }
>>    else
>>      {
>>        /* We do not have the parition. */
>> -      snprintf (disk, sizeof (disk), "/disk@1");
>> -      sysfs_path = block_device_get_sysfs_path_and_link (device);
>> +      sysfs_path = nvme_get_syspath (device);
>> +      if (strstr (sysfs_path, "nvme-fabrics"))
>> +        {
>> +          nvmeof_info = malloc (sizeof (struct ofpath_nvmeof_info));
>> +          if (!nvmeof_info)
>> +            return NULL;
>> +
>> +          ret_val = of_path_get_nvmeof_adapter_info (sysfs_path, 
>> nvmeof_info);
>> +          if (ret_val == -1)
>> +            {
>> +              free (nvmeof_info);
>> +              return NULL;
>> +            }
>> +
>> +          sysfs_path = of_find_fc_host (nvmeof_info->host_wwpn);
>> +          if (!sysfs_path)
>> +            {
>> +              free (nvmeof_info);
>> +              return NULL;
>> +            }
>> +
>> +          chars_written = snprintf (disk,sizeof(disk), 
>> "/nvme-of/controller@%s,%x:nqn=%s",
>> +                                    nvmeof_info->target_wwpn, 0xffff,
>> +                                    nvmeof_info->nqn);
>> +          unsigned int nsid = of_path_get_nvme_nsid (device);
>> +          if (nsid)
>> +            {
>> +              snprintf (disk+chars_written,sizeof("/namespace@") + 
>> sizeof(char) * MAX_NVME_NSID_DIGITS,
>> +                        "/namespace@%x", nsid);
>> +            }
>> +          free (nvmeof_info);
>> +        }
>> +      else
>> +        {
>> +          snprintf (disk, sizeof (disk), "/disk@1");
>> +        }
>>      }
>> 
>>    of_path = find_obppath (sysfs_path);
>> @@ -429,7 +785,6 @@ of_path_of_nvme(const char *sys_devname 
>> __attribute__((unused)),
>>    free (sysfs_path);
>>    return of_path;
>>  }
>> -#endif
>> 
>>  static int
>>  vendor_is_ATA(const char *path)
>> @@ -779,11 +1134,9 @@ grub_util_devname_to_ofpath (const char 
>> *sys_devname)
>>      /* All the models I've seen have a devalias "floppy".
>>         New models have no floppy at all. */
>>      ofpath = xstrdup ("floppy");
>> -#ifdef __sparc__
>>    else if (device[0] == 'n' && device[1] == 'v' && device[2] == 'm'
>>             && device[3] == 'e')
>>      ofpath = of_path_of_nvme (name_buf, device, devnode, devicenode);
>> -#endif
>>    else
>>      {
>>        grub_util_warn (_("unknown device type %s"), device);
>> diff --git a/include/grub/util/ofpath.h b/include/grub/util/ofpath.h
>> index b43c523..85172bc 100644
>> --- a/include/grub/util/ofpath.h
>> +++ b/include/grub/util/ofpath.h
>> @@ -3,4 +3,32 @@
>> 
>>  char *grub_util_devname_to_ofpath (const char *devname);
>> 
>> +struct ofpath_files_list_node
>> +{
>> +  char* filename;
>> +  struct ofpath_files_list_node* next;
>> +};
>> +
>> +struct ofpath_files_list_root
>> +{
>> +  int items;
>> +  struct ofpath_files_list_node* first;
>> +};
>> +
>> +struct ofpath_nvmeof_info
>> +{
>> +  char* host_wwpn;
>> +  char* target_wwpn;
>> +  char* nqn;
>> +  int cntlid;
>> +  int nsid;
>> +};
>> +
>> +int of_path_get_nvmeof_adapter_info (char* sysfs_path, struct 
>> ofpath_nvmeof_info* nvmeof_info);
>> +unsigned int of_path_get_nvme_nsid (const char* devname);
>> +int add_filename_to_pile (char *filename, struct 
>> ofpath_files_list_root* root);
>> +void find_file (char* filename, char* directory, struct 
>> ofpath_files_list_root* root, int max_depth, int depth);
>> +char* of_find_fc_host (char* host_wwpn);
>> +void free_ofpath_files_list (struct ofpath_files_list_root* root);
>> +
>>  #endif /* ! GRUB_OFPATH_MACHINE_UTIL_HEADER */
>> --
>> 2.31.1

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel