On Mon, Apr 25, 2016 at 03:30:47PM -0500, Serge E. Hallyn wrote:
> Quoting Seth Forshee (seth.forshee@xxxxxxxxxxxxx):
> > In a userns mount some on-disk inodes may have ids which do not
> > map into s_user_ns, in which case the in-kernel inodes are owned
> > by invalid users. The superblock owner should be able to change
> > attributes of these inodes but cannot. However it is unsafe to
> > grant the superblock owner privileged access to all inodes in the
> > superblock since proc, sysfs, etc. use DAC to protect files which
> > may not belong to s_user_ns. The problem is restricted to only
> > inodes where the owner or group is an invalid user.
> >
> > We can work around this by allowing users with CAP_CHOWN in
> > s_user_ns to change an invalid owner or group id, so long as the
> > other id is either invalid or mappable in s_user_ns. After
> > changing ownership the user will be privileged towards the inode
> > and thus able to change other attributes.
> >
> > As an precaution, checks for invalid ids are added to the proc
> > and kernfs setattr interfaces. These filesystems are not expected
> > to have inodes with invalid ids, but if it does happen any
> > setattr operations will return -EPERM.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@xxxxxxxxxxxxx>
>
> Acked-by: Serge Hallyn <serge.hallyn@xxxxxxxxxxxxx>
>
> bug a request below,
>
> > ---
> > fs/attr.c | 47 +++++++++++++++++++++++++++++++++++++++--------
> > fs/kernfs/inode.c | 2 ++
> > fs/proc/base.c | 2 ++
> > fs/proc/generic.c | 3 +++
> > fs/proc/proc_sysctl.c | 2 ++
> > 5 files changed, 48 insertions(+), 8 deletions(-)
> >
> > diff --git a/fs/attr.c b/fs/attr.c
> > index 3cfaaac4a18e..a8b0931654a5 100644
> > --- a/fs/attr.c
> > +++ b/fs/attr.c
> > @@ -16,6 +16,43 @@
> > #include <linux/evm.h>
> > #include <linux/ima.h>
> >
> > +static bool chown_ok(const struct inode *inode, kuid_t uid)
> > +{
> > + struct user_namespace *user_ns;
> > +
> > + if (uid_eq(current_fsuid(), inode->i_uid) && uid_eq(uid, inode->i_uid))
> > + return true;
> > + if (capable_wrt_inode_uidgid(inode, CAP_CHOWN))
> > + return true;
> > +
> > + user_ns = inode->i_sb->s_user_ns;
> > + if (!uid_valid(inode->i_uid) &&
> > + (!gid_valid(inode->i_gid) || kgid_has_mapping(user_ns, inode->i_gid)) &&
>
> This confused me to no end :) Perhaps a "is_unmapped_valid_gid()" helper
> would make it clearer what this is meant to do? Or else maybe a comment
> above chown_ok(), explaining that
>
> 1. for a blockdev, the uid is converted at inode read so that it is
> either mapped or invalid
> 2. for sysfs / etc, uid can be valid but not mapped into the userns
Even with a helper a comment is probably helpful to explain why. I'll do
that first, then see if a helper would make things any clearer.
Honestly, I had to think about the helper name you proposed for a minute
before it made sense even though I already understood the code ;-)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
