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 <[email protected]>
Signed-off-by: Andriy Gapon <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes #3506

3 files changed, 38 insertions, 8 deletions

diff --git a/module/zfs/fm.c b/module/zfs/fm.c
index b67a7076d..999bd8adc 100644
--- 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 4c0060b67..0eef257f1 100644
--- 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 18a0671a8..bc3892645 100644
--- 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