linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: ebiederm@xmission.com (Eric W. Biederman)
To: "Serge E. Hallyn" <serge@hallyn.com>
Cc: Seth Forshee <seth.forshee@canonical.com>,
	lkml <linux-kernel@vger.kernel.org>,
	linux-api@vger.kernel.org, linux-security-module@vger.kernel.org,
	Kees Cook <keescook@chromium.org>,
	Andreas Gruenbacher <agruenba@redhat.com>,
	Andy Lutomirski <luto@kernel.org>,
	"Andrew G. Morgan" <morgan@kernel.org>
Subject: Re: [PATCH] Introduce v3 namespaced file capabilities
Date: Thu, 27 Apr 2017 11:20:44 -0500	[thread overview]
Message-ID: <87r30d255v.fsf@xmission.com> (raw)
In-Reply-To: <87vapwncws.fsf@xmission.com> (Eric W. Biederman's message of "Sat, 22 Apr 2017 20:14:11 -0500")

ebiederm@xmission.com (Eric W. Biederman) writes:

> "Serge E. Hallyn" <serge@hallyn.com> writes:
>
>> Quoting Eric W. Biederman (ebiederm@xmission.com):
>>> 
>>> "Serge E. Hallyn" <serge@hallyn.com> writes:
>>> 
>>> > diff --git a/fs/xattr.c b/fs/xattr.c
>>> > index 7e3317c..75cc65a 100644
>>> > --- a/fs/xattr.c
>>> > +++ b/fs/xattr.c
>>> > @@ -170,12 +170,29 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
>>> >  		const void *value, size_t size, int flags)
>>> >  {
>>> >  	struct inode *inode = dentry->d_inode;
>>> > -	int error = -EAGAIN;
>>> > +	int error;
>>> > +	void *wvalue = NULL;
>>> > +	size_t wsize = 0;
>>> >  	int issec = !strncmp(name, XATTR_SECURITY_PREFIX,
>>> >  				   XATTR_SECURITY_PREFIX_LEN);
>>> >  
>>> > -	if (issec)
>>> > +	if (issec) {
>>> >  		inode->i_flags &= ~S_NOSEC;
>>> > +
>>> > +		if (!strcmp(name, "security.capability")) {
>>> > +			error = cap_setxattr_convert_nscap(dentry, value, size,
>>> > +					&wvalue, &wsize);
>>> > +			if (error < 0)
>>> > +				return error;
>>> > +			if (wvalue) {
>>> > +				value = wvalue;
>>> > +				size = wsize;
>>> > +			}
>>> > +		}
>>> > +	}
>>> > +
>>> > +	error = -EAGAIN;
>>> > +
>>> 
>>> Why is the conversion in __vfs_setxattr_noperm and not in setattr as
>>> was done for posix_acl_fix_xattr_from_user?
>>
>> I think I was thinking I wanted to catch all the vfs_setxattr operations,
>> but I don't think that's right.  Moving to setxattr seems right.  I'll
>> look around a bit more.
>
> Thanks.  This is one of these little details that we want a good answer
> to why there.  If you can document that in your patch description when
> you resend I would appreciate it.

Ok. Grrr.

Looking at this a little more getting it correct where we call the
conversion operation is critical. 

I believe the current placement of cap_setxattr_convert_nscap is
actively wrong.  In particular unless I am misleading something this
will trigger multiple conversions when setting one of these attributes
on overlayfs.

The stragey I adopted for for posix acls is:

   On a write from userspace convert from current_user_ns() to &init_user_ns.
   On a write to the filesystem convert from &init_user_ns to fs_user_ns.
   
   On a read from the filesystem convert from fs_user_ns to &init_user_ns
   On a read from the kernel to userspace convert from &init_user_ns
      to current_user_ns().

Overall a good strategy but no one we can trivially adopt for the
capability xattr as the second write to filesystem method does not
appear to actually exist for anything except for posix acls.

I need to think a little more about how we want to accomplish this for
the capability xattr.  My apoligies for leading you down a path that has
all of these bumps and then being sufficiently distracted not to help
you through this maze.

The only easy solution I can see is to just always keep things in
&init_user_ns inside the kernel.   That works until we bring fuse or
other unprivileged mounts onboard that have storage outside of the
kernel.

Seth and I will have to rework that for fuse support but that sounds
better than not letting such an issue prevent us from merging the code.


Eric

  reply	other threads:[~2017-04-27 16:27 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-19 16:48 [PATCH] Introduce v3 namespaced file capabilities Serge E. Hallyn
2017-04-21 21:37 ` Eric W. Biederman
2017-04-21 21:50   ` Serge E. Hallyn
2017-04-21 21:53     ` Eric W. Biederman
2017-04-21 23:58 ` Eric W. Biederman
2017-04-22 15:14   ` Serge E. Hallyn
2017-04-23  1:14     ` Eric W. Biederman
2017-04-27 16:20       ` Eric W. Biederman [this message]
2017-04-27 16:52         ` Serge E. Hallyn
2017-04-27 17:00           ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r30d255v.fsf@xmission.com \
    --to=ebiederm@xmission.com \
    --cc=agruenba@redhat.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=morgan@kernel.org \
    --cc=serge@hallyn.com \
    --cc=seth.forshee@canonical.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).