Skip to content

Commit

Permalink
Want /dev/zfs umask module parameter
Browse files Browse the repository at this point in the history 
We introduce a module parameter that enforces a umask on /dev/zfs. It
was originally meant to be set only at module load time, but some
difficulties with `modprobe zfs zfsdev_umask=777` in local testing lead
me to allow its owner (default root) to change it at runtime. This
should ease the transition for those that rely on this behavior,
provided that we state in the release notes that future releases are
expected to remove the ability to modify it at runtime. When the cause
of that modprobe command failing to set the module is found, we can
tighten the permissions for a future release.

Closes openzfs#4100

Signed-off-by: Richard Yao <[email protected]>
  • Loading branch information
@ryao
ryao committed Dec 17, 2015
1 parent 799402d commit 3fb2782
Showing 1 changed file with 32 additions and 0 deletions.
32 changes: 32 additions & 0 deletions module/zfs/zfs_ioctl.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,9 @@
kmutex_t zfsdev_state_lock;
zfsdev_state_t *zfsdev_state_list;

/* The bits are stored in decimal format, not octal */
int zfsdev_umask = 77;

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Dec 17, 2015

I'd suggest using octal here, I think it's what users will expect when working with a umask. The kernel helpers should understand octal so I don't expect any problems, but I haven't verified this.

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Dec 17, 2015

This should be entirely disabled by default to avoid user confusion. This is a work around, we should be able to trust the permission bits on the device under normal circumstances.

This comment has been minimized.

Sign in to view

Copy link
@ryao

ryao Dec 17, 2015

Author Owner

The purpose of a mitigation is to prevent threats before they happen. If we disable it by default, there is practically no protection. Would you be willing to accept 0007 (octal notation) as the default? The main danger is allowing other to do anything it wants.

As for octal, anyone reading /sys/module/zfs/parameter/zfsdev_umask will see it in decimal and it will not make sense. If you are alright with that, we can go with octal.

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Dec 18, 2015

I think 0007 is a reasonable compromise. That should prevent us from breaking lots of systems when this is merged. I'm also okay with the zfsdev_umask output, there's not much we can do about it and frankly most people shouldn't be touching this in the first place.

extern void zfs_init(void);
extern void zfs_fini(void);

Expand Down Expand Up @@ -5776,6 +5779,29 @@ zfsdev_release(struct inode *ino, struct file *filp)
return (-error);
}

boolean_t
zfsdev_fail_cred_check(struct file *filp)

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Dec 17, 2015

Let's make this static to make it clear this is a private function.

This comment has been minimized.

Sign in to view

Copy link
@ryao

ryao Dec 17, 2015

Author Owner

Okay.
{
umode_t umode = filp->f_inode->i_mode;

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Dec 17, 2015

Use file_inode() here it's the preferred upstream interface and a compat wrapper is already provided for older kernels.

This comment has been minimized.

Sign in to view

Copy link
@ryao

ryao Dec 17, 2015

Author Owner

I was on the fence on whether to go with the file pointer or pass the mode and credentials individually. I think I will do the latter so that this function is less Linux-specific. I will use file_inode in zfsdev_ioctl.

if (!(filp->f_mode & (FMODE_WRITE|FMODE_READ)))
return (B_TRUE);

/* Check other credential */
if (((~zfsdev_umask & umode) & 8) >= 6)
return (B_FALSE);

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Dec 17, 2015

Magic numbers should be avoided. When doing any operations with the mode bits make sure you use the S_* macros in linux/stat.h to mask out the bit you need and do comparisons.

This comment has been minimized.

Sign in to view

Copy link
@ryao

ryao Dec 17, 2015

Author Owner

Okay.

/* Check group credential */
if (((~(zfsdev_umask / 10) & (umode >> 4)) & 8) >= 6)
return (B_FALSE);

/* Check user credential */
if (((~(zfsdev_umask / 100) & (umode >> 8)) & 8) >= 6)
return (B_FALSE);

return (B_TRUE);
}

static long
zfsdev_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
{
Expand All @@ -5787,6 +5813,9 @@ zfsdev_ioctl(struct file *filp, unsigned cmd, unsigned long arg)
nvlist_t *innvl = NULL;
fstrans_cookie_t cookie;

if (zfsdev_fail_cred_check(filp))

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Dec 17, 2015

The logic feels backwards to me, I think a zfsdev_cred_check() function would be clearer and then we check for a failure of that function.

This comment has been minimized.

Sign in to view

Copy link
@ryao

ryao Dec 17, 2015

Author Owner

Okay.
return (-SET_ERROR(EPERM));

vecnum = cmd - ZFS_IOC_FIRST;
if (vecnum >= sizeof (zfs_ioc_vec) / sizeof (zfs_ioc_vec[0]))
return (-SET_ERROR(EINVAL));
Expand Down Expand Up @@ -6075,6 +6104,9 @@ _fini(void)
module_init(_init);
module_exit(_fini);

module_param(zfsdev_umask, int, 0644);
MODULE_PARM_DESC(zfsdev_umask, "/dev/zfs umask");

This comment has been minimized.

Sign in to view

Copy link
@behlendorf

behlendorf Dec 18, 2015

When you refresh this please also make sure you add a section to the zfs-module-parameters.5 man page.

MODULE_DESCRIPTION("ZFS");
MODULE_AUTHOR(ZFS_META_AUTHOR);
MODULE_LICENSE(ZFS_META_LICENSE);
Expand Down

0 comments on commit 3fb2782

Please sign in to comment.