[dm-devel] [PATCH 05/14] libmultipath: sanitize error checking in sysfs accessors

[mwilck at suse.com]

From: Martin Wilck <mwilck at suse.com>

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 <mwilck at suse.com>
---
 libmultipath/sysfs.c | 94 ++++++++++++++++++--------------------------
 1 file changed, 39 insertions(+), 55 deletions(-)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 4db911c..1f0f207 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));
+		condlog(2, "%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 -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;
 	}
 
-- 
2.36.1