On Fri, May 1, 2020 at 9:31 AM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> On Fri, May 01, 2020 at 05:14:44AM +0100, Al Viro wrote:
> > On Thu, Apr 09, 2020 at 11:28:59PM +0200, Miklos Szeredi wrote:
> > > From: Miklos Szeredi <mszeredi@xxxxxxxxxx>
> > >
> > > Whiteouts, unlike real device node should not require privileges to create.
> > >
> > > The general concern with device nodes is that opening them can have side
> > > effects. The kernel already avoids zero major (see
> > > Documentation/admin-guide/devices.txt). To be on the safe side the patch
> > > explicitly forbids registering a char device with 0/0 number (see
> > > cdev_add()).
> > >
> > > This guarantees that a non-O_PATH open on a whiteout will fail with ENODEV;
> > > i.e. it won't have any side effect.
> >
> > > int vfs_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t dev)
> > > {
> > > + bool is_whiteout = S_ISCHR(mode) && dev == WHITEOUT_DEV;
> > > int error = may_create(dir, dentry);
> > >
> > > if (error)
> > > return error;
> > >
> > > - if ((S_ISCHR(mode) || S_ISBLK(mode)) && !capable(CAP_MKNOD))
> > > + if ((S_ISCHR(mode) || S_ISBLK(mode)) && !capable(CAP_MKNOD) &&
> > > + !is_whiteout)
> > > return -EPERM;
> >
> > Hmm... That exposes vfs_whiteout() to LSM; are you sure that you won't
> > end up with regressions for overlayfs on sufficiently weird setups?
>
> You're right. OTOH, what can we do? We can't fix the weird setups, only the
> distros/admins can.
>
> Can we just try this, and revert to calling ->mknod directly from overlayfs if
> it turns out to be a problem that people can't fix easily?
>
> I guess we could add a new ->whiteout security hook as well, but I'm not sure
> it's worth it. Cc: LMS mailing list; patch re-added for context.
>
> Thanks,
> Miklos
>
> ---
> fs/char_dev.c | 3 +++
> fs/namei.c | 17 ++++-------------
> include/linux/device_cgroup.h | 3 +++
> 3 files changed, 10 insertions(+), 13 deletions(-)
>
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -483,6 +483,9 @@ int cdev_add(struct cdev *p, dev_t dev,
> p->dev = dev;
> p->count = count;
>
> + if (WARN_ON(dev == WHITEOUT_DEV))
> + return -EBUSY;
> +
> error = kobj_map(cdev_map, dev, count, NULL,
> exact_match, exact_lock, p);
> if (error)
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3505,12 +3505,14 @@ EXPORT_SYMBOL(user_path_create);
>
> int vfs_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t dev)
> {
> + bool is_whiteout = S_ISCHR(mode) && dev == WHITEOUT_DEV;
> int error = may_create(dir, dentry);
>
> if (error)
> return error;
>
> - if ((S_ISCHR(mode) || S_ISBLK(mode)) && !capable(CAP_MKNOD))
> + if ((S_ISCHR(mode) || S_ISBLK(mode)) && !capable(CAP_MKNOD) &&
> + !is_whiteout)
Sorry for sidetracking, but !capable(CAP_MKNOD) needs to be last in
the chain, otherwise you could get a bogus audit report of CAP_MKNOD
being denied in case is_whiteout is true.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
