On Wed, Jul 06, 2022 at 04:38:13PM +0200, mwilck@xxxxxxxx wrote:
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
> 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@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
