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

[Benjamin Marzinski <bmarzins@redhat.com>]

On Wed, Jul 06, 2022 at 04:38:13PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@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@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));

You log at loglevel 2 here, but at 3 for an open() failure in
__sysfs_attr_get_value(). However I notice that this gets resolved in
PATCH 8/14, so it's no big deal.

-Ben

>  		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
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel