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 <dieg...@br.ibm.com>
> Signed-off-by: Avnish Chouhan <avn...@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
