[PATCH] vfs: allow unprivileged whiteout creation

[Ondrej Mosnacek]

On Fri, May 1, 2020 at 9:31 AM Miklos Szeredi <miklos at szeredi.hu> 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 at redhat.com>
> > >
> > > 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.