linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vfs: allow unprivileged whiteout creation
@ 2020-04-09 21:28 Miklos Szeredi
  2020-05-01  4:14 ` Al Viro
  0 siblings, 1 reply; 8+ messages in thread
From: Miklos Szeredi @ 2020-04-09 21:28 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel

From: Miklos Szeredi <mszeredi@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.

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 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)
 		return -EPERM;
 
 	if (!dir->i_op->mknod)
@@ -4345,9 +4347,6 @@ static int do_renameat2(int olddfd, cons
 	    (flags & RENAME_EXCHANGE))
 		return -EINVAL;
 
-	if ((flags & RENAME_WHITEOUT) && !capable(CAP_MKNOD))
-		return -EPERM;
-
 	if (flags & RENAME_EXCHANGE)
 		target_flags = 0;
 
@@ -4485,15 +4484,7 @@ SYSCALL_DEFINE2(rename, const char __use
 
 int vfs_whiteout(struct inode *dir, struct dentry *dentry)
 {
-	int error = may_create(dir, dentry);
-	if (error)
-		return error;
-
-	if (!dir->i_op->mknod)
-		return -EPERM;
-
-	return dir->i_op->mknod(dir, dentry,
-				S_IFCHR | WHITEOUT_MODE, WHITEOUT_DEV);
+	return vfs_mknod(dir, dentry, S_IFCHR | WHITEOUT_MODE, WHITEOUT_DEV);
 }
 EXPORT_SYMBOL(vfs_whiteout);
 
--- a/include/linux/device_cgroup.h
+++ b/include/linux/device_cgroup.h
@@ -51,6 +51,9 @@ static inline int devcgroup_inode_mknod(
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))
 		return 0;
 
+	if (S_ISCHR(mode) && dev == WHITEOUT_DEV)
+		return 0;
+
 	if (S_ISBLK(mode))
 		type = DEVCG_DEV_BLOCK;
 	else

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] vfs: allow unprivileged whiteout creation
  2020-04-09 21:28 [PATCH] vfs: allow unprivileged whiteout creation Miklos Szeredi
@ 2020-05-01  4:14 ` Al Viro
  2020-05-01  7:31   ` Miklos Szeredi
  0 siblings, 1 reply; 8+ messages in thread
From: Al Viro @ 2020-05-01  4:14 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: linux-fsdevel

On Thu, Apr 09, 2020 at 11:28:59PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <mszeredi@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?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] vfs: allow unprivileged whiteout creation
  2020-05-01  4:14 ` Al Viro
@ 2020-05-01  7:31   ` Miklos Szeredi
  2020-05-01 14:46     ` Ondrej Mosnacek
  2020-05-01 18:39     ` Stephen Smalley
  0 siblings, 2 replies; 8+ messages in thread
From: Miklos Szeredi @ 2020-05-01  7:31 UTC (permalink / raw)
  To: Al Viro; +Cc: linux-fsdevel, linux-security-module

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@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)
 		return -EPERM;
 
 	if (!dir->i_op->mknod)
@@ -4345,9 +4347,6 @@ static int do_renameat2(int olddfd, cons
 	    (flags & RENAME_EXCHANGE))
 		return -EINVAL;
 
-	if ((flags & RENAME_WHITEOUT) && !capable(CAP_MKNOD))
-		return -EPERM;
-
 	if (flags & RENAME_EXCHANGE)
 		target_flags = 0;
 
@@ -4485,15 +4484,7 @@ SYSCALL_DEFINE2(rename, const char __use
 
 int vfs_whiteout(struct inode *dir, struct dentry *dentry)
 {
-	int error = may_create(dir, dentry);
-	if (error)
-		return error;
-
-	if (!dir->i_op->mknod)
-		return -EPERM;
-
-	return dir->i_op->mknod(dir, dentry,
-				S_IFCHR | WHITEOUT_MODE, WHITEOUT_DEV);
+	return vfs_mknod(dir, dentry, S_IFCHR | WHITEOUT_MODE, WHITEOUT_DEV);
 }
 EXPORT_SYMBOL(vfs_whiteout);
 
--- a/include/linux/device_cgroup.h
+++ b/include/linux/device_cgroup.h
@@ -51,6 +51,9 @@ static inline int devcgroup_inode_mknod(
 	if (!S_ISBLK(mode) && !S_ISCHR(mode))
 		return 0;
 
+	if (S_ISCHR(mode) && dev == WHITEOUT_DEV)
+		return 0;
+
 	if (S_ISBLK(mode))
 		type = DEVCG_DEV_BLOCK;
 	else

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] vfs: allow unprivileged whiteout creation
  2020-05-01  7:31   ` Miklos Szeredi
@ 2020-05-01 14:46     ` Ondrej Mosnacek
  2020-05-05 10:50       ` Miklos Szeredi
  2020-05-01 18:39     ` Stephen Smalley
  1 sibling, 1 reply; 8+ messages in thread
From: Ondrej Mosnacek @ 2020-05-01 14:46 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Al Viro, linux-fsdevel, Linux Security Module list

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] vfs: allow unprivileged whiteout creation
  2020-05-01  7:31   ` Miklos Szeredi
  2020-05-01 14:46     ` Ondrej Mosnacek
@ 2020-05-01 18:39     ` Stephen Smalley
  2020-05-04 11:18       ` Miklos Szeredi
  1 sibling, 1 reply; 8+ messages in thread
From: Stephen Smalley @ 2020-05-01 18:39 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Al Viro, Linux FS Devel, LSM List

On Fri, May 1, 2020 at 3:34 AM Miklos Szeredi <miklos@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@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.

I feel like I am still missing context but IIUC this change is
allowing unprivileged userspace to explicitly call mknod(2) with the
whiteout device number and skip all permission checks (except the LSM
one). And then you are switching vfs_whiteout() over to using
vfs_mknod() internally since it no longer does permission checking and
that was why vfs_whiteout() was separate originally to avoid imposing
any checks on overlayfs-internal creation of whiteouts?

If that's correct, then it seems problematic since we have no way in
the LSM hook to distinguish the two cases (userspace invocation of
mknod(2) versus overlayfs-internal operation).  Don't know offhand
what credential is in effect in the overlayfs case (mounter or
current) but regardless Android seems to use current regardless, and
that could easily fail.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] vfs: allow unprivileged whiteout creation
  2020-05-01 18:39     ` Stephen Smalley
@ 2020-05-04 11:18       ` Miklos Szeredi
  2020-05-04 15:38         ` Stephen Smalley
  0 siblings, 1 reply; 8+ messages in thread
From: Miklos Szeredi @ 2020-05-04 11:18 UTC (permalink / raw)
  To: Stephen Smalley; +Cc: Al Viro, Linux FS Devel, LSM List

On Fri, May 1, 2020 at 8:40 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Fri, May 1, 2020 at 3:34 AM Miklos Szeredi <miklos@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@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.
>
> I feel like I am still missing context but IIUC this change is
> allowing unprivileged userspace to explicitly call mknod(2) with the
> whiteout device number and skip all permission checks (except the LSM
> one). And then you are switching vfs_whiteout() over to using
> vfs_mknod() internally since it no longer does permission checking and
> that was why vfs_whiteout() was separate originally to avoid imposing
> any checks on overlayfs-internal creation of whiteouts?
>
> If that's correct, then it seems problematic since we have no way in
> the LSM hook to distinguish the two cases (userspace invocation of
> mknod(2) versus overlayfs-internal operation).  Don't know offhand
> what credential is in effect in the overlayfs case (mounter or
> current) but regardless Android seems to use current regardless, and
> that could easily fail.

The major point is: whiteouts are *not* device files, not in the real
sense, it just happens that whiteouts are represented by the file
having a char/0/0 type.

Also the fact that overlayfs invocation is indistinguishable from
userspace invocation is very much on purpose.  Whiteout creation was
the exception before this change, not the rule.

If you consider the above, how should this be handled from an LSM perspective?

Thanks,
Miklos

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] vfs: allow unprivileged whiteout creation
  2020-05-04 11:18       ` Miklos Szeredi
@ 2020-05-04 15:38         ` Stephen Smalley
  0 siblings, 0 replies; 8+ messages in thread
From: Stephen Smalley @ 2020-05-04 15:38 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Al Viro, Linux FS Devel, LSM List

On Mon, May 4, 2020 at 7:18 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, May 1, 2020 at 8:40 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Fri, May 1, 2020 at 3:34 AM Miklos Szeredi <miklos@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@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.
> >
> > I feel like I am still missing context but IIUC this change is
> > allowing unprivileged userspace to explicitly call mknod(2) with the
> > whiteout device number and skip all permission checks (except the LSM
> > one). And then you are switching vfs_whiteout() over to using
> > vfs_mknod() internally since it no longer does permission checking and
> > that was why vfs_whiteout() was separate originally to avoid imposing
> > any checks on overlayfs-internal creation of whiteouts?
> >
> > If that's correct, then it seems problematic since we have no way in
> > the LSM hook to distinguish the two cases (userspace invocation of
> > mknod(2) versus overlayfs-internal operation).  Don't know offhand
> > what credential is in effect in the overlayfs case (mounter or
> > current) but regardless Android seems to use current regardless, and
> > that could easily fail.
>
> The major point is: whiteouts are *not* device files, not in the real
> sense, it just happens that whiteouts are represented by the file
> having a char/0/0 type.
>
> Also the fact that overlayfs invocation is indistinguishable from
> userspace invocation is very much on purpose.  Whiteout creation was
> the exception before this change, not the rule.
>
> If you consider the above, how should this be handled from an LSM perspective?

In that case, I guess you can leave the patch as is aside from moving
the capable() check last, and we will just need to allow creation of
these files to the mounter context for overlayfs-internal usage. It
doesn't appear to be safe to skip the hook call altogether for the
general case (e.g. userspace mknod(2)).

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] vfs: allow unprivileged whiteout creation
  2020-05-01 14:46     ` Ondrej Mosnacek
@ 2020-05-05 10:50       ` Miklos Szeredi
  0 siblings, 0 replies; 8+ messages in thread
From: Miklos Szeredi @ 2020-05-05 10:50 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: Al Viro, linux-fsdevel, Linux Security Module list

On Fri, May 1, 2020 at 4:46 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Fri, May 1, 2020 at 9:31 AM Miklos Szeredi <miklos@szeredi.hu> wrote:

> > --- 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.

Thanks, fixed in the latest revision.

Miklos

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2020-05-05 10:51 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-09 21:28 [PATCH] vfs: allow unprivileged whiteout creation Miklos Szeredi
2020-05-01  4:14 ` Al Viro
2020-05-01  7:31   ` Miklos Szeredi
2020-05-01 14:46     ` Ondrej Mosnacek
2020-05-05 10:50       ` Miklos Szeredi
2020-05-01 18:39     ` Stephen Smalley
2020-05-04 11:18       ` Miklos Szeredi
2020-05-04 15:38         ` Stephen Smalley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).