zfsdev_getminor() should check for invalid file handles

Unit testing at ClusterHQ found that passing an invalid file handle to
zfs_ioc_hold results in a NULL pointer dereference on a system without
assertions:

IP: [<ffffffffa0218aa0>] zfsdev_getminor+0x10/0x20 [zfs]
Call Trace:
[<ffffffffa021b4b0>] zfs_onexit_fd_hold+0x20/0x40 [zfs]
[<ffffffffa0214043>] zfs_ioc_hold+0x93/0xd0 [zfs]
[<ffffffffa0215890>] zfsdev_ioctl+0x200/0x500 [zfs]

An assertion would have caught this had they been enabled, but this is
something that the kernel module should handle without failing.  We
resolve this by searching the linked list to ensure that the file
handle's private_data points to a valid zfsdev_state_t.

Signed-off-by: Richard Yao <ryao@gentoo.org>
Signed-off-by: Andriy Gapon <avg@FreeBSD.org>
Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Closes #3506

diff --git a/include/sys/zfs_ioctl.h b/include/sys/zfs_ioctl.h
index c71ceb9c5a57b74b28ab5f2e4b36a2de9e7d2743..09a96c043bf0e234f7973fc051f535d215135616 100644 (file)
--- a/include/sys/zfs_ioctl.h
+++ b/include/sys/zfs_ioctl.h
@@ -407,7 +407,7 @@ typedef struct zfsdev_state {
 } zfsdev_state_t;
 
 extern void *zfsdev_get_state(minor_t minor, enum zfsdev_state_type which);
-extern minor_t zfsdev_getminor(struct file *filp);
+extern int zfsdev_getminor(struct file *filp, minor_t *minorp);
 extern minor_t zfsdev_minor_alloc(void);
 
 #endif /* _KERNEL */

diff --git a/module/zfs/fm.c b/module/zfs/fm.c
index b67a7076dc0be35ac1fda0597d3f43e75d7a56ef..999bd8adc5187efff9a2747129296dffe3e2aaf6 100644 (file)
--- a/module/zfs/fm.c
+++ b/module/zfs/fm.c
@@ -593,8 +593,9 @@ zfs_zevent_fd_hold(int fd, minor_t *minorp, zfs_zevent_t **ze)
        if (fp == NULL)
                return (EBADF);
 
-       *minorp = zfsdev_getminor(fp->f_file);
-       error = zfs_zevent_minor_to_state(*minorp, ze);
+       error = zfsdev_getminor(fp->f_file, minorp);
+       if (error == 0)
+               error = zfs_zevent_minor_to_state(*minorp, ze);
 
        if (error)
                zfs_zevent_fd_rele(fd);

diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
index 4c0060b67bb7d9d65ba3ffc9f0382d3411a25af8..0eef257f13c789d9c9a148e0976b28d34011d236 100644 (file)
--- a/module/zfs/zfs_ioctl.c
+++ b/module/zfs/zfs_ioctl.c
@@ -5687,13 +5687,35 @@ zfsdev_get_state(minor_t minor, enum zfsdev_state_type which)
        return (ptr);
 }
 
-minor_t
-zfsdev_getminor(struct file *filp)
+int
+zfsdev_getminor(struct file *filp, minor_t *minorp)
 {
+       zfsdev_state_t *zs, *fpd;
+
        ASSERT(filp != NULL);
-       ASSERT(filp->private_data != NULL);
+       ASSERT(!MUTEX_HELD(&zfsdev_state_lock));
+
+       fpd = filp->private_data;
+       if (fpd == NULL)
+               return (EBADF);
+
+       mutex_enter(&zfsdev_state_lock);
+
+       for (zs = zfsdev_state_list; zs != NULL; zs = zs->zs_next) {
+
+               if (zs->zs_minor == -1)
+                       continue;
+
+               if (fpd == zs) {
+                       *minorp = fpd->zs_minor;
+                       mutex_exit(&zfsdev_state_lock);
+                       return (0);
+               }
+       }
+
+       mutex_exit(&zfsdev_state_lock);
 
-       return (((zfsdev_state_t *)filp->private_data)->zs_minor);
+       return (EBADF);
 }
 
 /*

diff --git a/module/zfs/zfs_onexit.c b/module/zfs/zfs_onexit.c
index 18a0671a86f698fc6750febf040d259979673f86..bc3892645fe1071db24670e46a22483c62e86931 100644 (file)
--- a/module/zfs/zfs_onexit.c
+++ b/module/zfs/zfs_onexit.c
@@ -126,13 +126,20 @@ zfs_onexit_fd_hold(int fd, minor_t *minorp)
 {
        file_t *fp;
        zfs_onexit_t *zo;
+       int error;
 
        fp = getf(fd);
        if (fp == NULL)
                return (SET_ERROR(EBADF));
 
-       *minorp = zfsdev_getminor(fp->f_file);
-       return (zfs_onexit_minor_to_state(*minorp, &zo));
+       error = zfsdev_getminor(fp->f_file, minorp);
+       if (error == 0)
+               error = zfs_onexit_minor_to_state(*minorp, &zo);
+
+       if (error)
+               zfs_onexit_fd_rele(fd);
+
+       return (error);
 }
 
 void