Skip to content

Commit

Permalink
zfsdev_getminor() should check for invalid file handles
Browse files Browse the repository at this point in the history
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 openzfs#3506
  • Loading branch information
ryao authored and behlendorf committed Jun 23, 2015
1 parent 99b14de commit 72540ea
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 9 deletions.
2 changes: 1 addition & 1 deletion include/sys/zfs_ioctl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down
5 changes: 3 additions & 2 deletions module/zfs/fm.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
30 changes: 26 additions & 4 deletions module/zfs/zfs_ioctl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/*
Expand Down
11 changes: 9 additions & 2 deletions module/zfs/zfs_onexit.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 72540ea

Please sign in to comment.