From 72540ea3148a2bc03860d7d59b2b5fdc9a5cdee7 Mon Sep 17 00:00:00 2001 From: Richard Yao Date: Thu, 16 Apr 2015 09:20:02 -0400 Subject: [PATCH] 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: [] zfsdev_getminor+0x10/0x20 [zfs] Call Trace: [] zfs_onexit_fd_hold+0x20/0x40 [zfs] [] zfs_ioc_hold+0x93/0xd0 [zfs] [] 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 Signed-off-by: Andriy Gapon Signed-off-by: Brian Behlendorf Closes #3506 --- include/sys/zfs_ioctl.h | 2 +- module/zfs/fm.c | 5 +++-- module/zfs/zfs_ioctl.c | 32 +++++++++++++++++++++++++++----- module/zfs/zfs_onexit.c | 11 +++++++++-- 4 files changed, 40 insertions(+), 10 deletions(-) diff --git a/include/sys/zfs_ioctl.h b/include/sys/zfs_ioctl.h index c71ceb9c5a57..09a96c043bf0 100644 --- 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 b67a7076dc0b..999bd8adc518 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 4c0060b67bb7..0eef257f13c7 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) { - ASSERT(filp != NULL); - ASSERT(filp->private_data != NULL); + zfsdev_state_t *zs, *fpd; - return (((zfsdev_state_t *)filp->private_data)->zs_minor); + ASSERT(filp != 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 (EBADF); } /* diff --git a/module/zfs/zfs_onexit.c b/module/zfs/zfs_onexit.c index 18a0671a86f6..bc3892645fe1 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