From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Martin Wilck Date: Tue, 5 Jul 2022 11:41:40 +0200 Subject: [PATCH] libmultipath: sanitize error checking in sysfs accessors udev_device_get_syspath() may return NULL; check for it, and check for pathname overflow. Disallow a zero buffer length. The fstat() calls were superfluous, as a read() or write() on the fd would return the respective error codes depending on file type or permissions, the extra system call and code complexity adds no value. Log levels should be moderate in sysfs.c, because it depends on the caller whether errors getting/setting certain attributes are fatal. Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski Signed-off-by: Benjamin Marzinski --- libmultipath/sysfs.c | 94 ++++++++++++++++++-------------------------- 1 file changed, 39 insertions(+), 55 deletions(-) diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c index 4db911cc..1f0f2074 100644 --- a/libmultipath/sysfs.c +++ b/libmultipath/sysfs.c @@ -47,46 +47,38 @@ static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char *attr_name, char *value, size_t value_len, bool binary) { + const char *syspath; char devpath[PATH_SIZE]; - struct stat statbuf; int fd; ssize_t size = -1; - if (!dev || !attr_name || !value) - return 0; + if (!dev || !attr_name || !value || !value_len) { + condlog(1, "%s: invalid parameters", __func__); + return -EINVAL; + } - snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev), - attr_name); + syspath = udev_device_get_syspath(dev); + if (!syspath) { + condlog(3, "%s: invalid udevice", __func__); + return -EINVAL; + } + if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) { + condlog(3, "%s: devpath overflow", __func__); + return -EOVERFLOW; + } condlog(4, "open '%s'", devpath); /* read attribute value */ fd = open(devpath, O_RDONLY); if (fd < 0) { - condlog(4, "attribute '%s' can not be opened: %s", - devpath, strerror(errno)); + condlog(3, "%s: attribute '%s' can not be opened: %s", + __func__, devpath, strerror(errno)); return -errno; } - if (fstat(fd, &statbuf) < 0) { - condlog(4, "stat '%s' failed: %s", devpath, strerror(errno)); - close(fd); - return -ENXIO; - } - /* skip directories */ - if (S_ISDIR(statbuf.st_mode)) { - condlog(4, "%s is a directory", devpath); - close(fd); - return -EISDIR; - } - /* skip non-writeable files */ - if ((statbuf.st_mode & S_IRUSR) == 0) { - condlog(4, "%s is not readable", devpath); - close(fd); - return -EPERM; - } - size = read(fd, value, value_len); if (size < 0) { - condlog(4, "read from %s failed: %s", devpath, strerror(errno)); size = -errno; + condlog(3, "%s: read from %s failed: %s", __func__, devpath, + strerror(errno)); if (!binary) value[0] = '\0'; } else if (!binary && size == (ssize_t)value_len) { @@ -119,51 +111,43 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device *dev, const char *attr_name, ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name, const char * value, size_t value_len) { + const char *syspath; char devpath[PATH_SIZE]; - struct stat statbuf; int fd; ssize_t size = -1; - if (!dev || !attr_name || !value || !value_len) - return 0; + if (!dev || !attr_name || !value || !value_len) { + condlog(1, "%s: invalid parameters", __func__); + return -EINVAL; + } + + syspath = udev_device_get_syspath(dev); + if (!syspath) { + condlog(3, "%s: invalid udevice", __func__); + return -EINVAL; + } + if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) { + condlog(3, "%s: devpath overflow", __func__); + return -EOVERFLOW; + } - snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev), - attr_name); condlog(4, "open '%s'", devpath); /* write attribute value */ fd = open(devpath, O_WRONLY); if (fd < 0) { - condlog(4, "attribute '%s' can not be opened: %s", - devpath, strerror(errno)); - return -errno; - } - if (fstat(fd, &statbuf) != 0) { - condlog(4, "stat '%s' failed: %s", devpath, strerror(errno)); - close(fd); + condlog(2, "%s: attribute '%s' can not be opened: %s", + __func__, devpath, strerror(errno)); return -errno; } - /* skip directories */ - if (S_ISDIR(statbuf.st_mode)) { - condlog(4, "%s is a directory", devpath); - close(fd); - return -EISDIR; - } - - /* skip non-writeable files */ - if ((statbuf.st_mode & S_IWUSR) == 0) { - condlog(4, "%s is not writeable", devpath); - close(fd); - return -EPERM; - } - size = write(fd, value, value_len); if (size < 0) { - condlog(4, "write to %s failed: %s", devpath, strerror(errno)); size = -errno; + condlog(3, "%s: write to %s failed: %s", __func__, + devpath, strerror(errno)); } else if (size < (ssize_t)value_len) { - condlog(4, "tried to write %ld to %s. Wrote %ld", - (long)value_len, devpath, (long)size); + condlog(3, "%s: underflow writing %zu bytes to %s. Wrote %zd bytes", + __func__, value_len, devpath, size); size = 0; }