From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman)
To: "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>
Cc: Seth Forshee
<seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>,
lkml <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
Andreas Gruenbacher
<agruenba-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>,
Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
"Andrew G. Morgan"
<morgan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Subject: Re: [PATCH] Introduce v3 namespaced file capabilities
Date: Sat, 22 Apr 2017 20:14:11 -0500 [thread overview]
Message-ID: <87vapwncws.fsf@xmission.com> (raw)
In-Reply-To: <20170422151412.GA14861-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org> (Serge E. Hallyn's message of "Sat, 22 Apr 2017 10:14:12 -0500")
"Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
> Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
>>
>> "Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org> writes:
>>
>> Overall this looks quite reasonable.
>>
>> My only big concern was the lack of verifying of magic_etc. As without
>
> Yes, I was relying too much on the size check.
>
>> that the code might not be future compatible with new versions of the
>> capability xattrs. It it tends to be nice to be able to boot old
>> kernels when regression testing etc. Even if they can't make use of
>> all of the features of a new filesystem.
>
> That certainly was my intent.
>
>> > 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.
>> Missing version checks on the magic_etc field.
>> And the wrong error code when the code deliberately refuses to return a
>> capability.
>
> Thanks, all looks good. Did you want to just squash these yourself and
> move on, keep them as separate patches, or have me incorporate into mine
> and resend?
Given that there is an outstanding question I would appreciate a resend
with an updated patch description, the changes squashed and possibly a
change in where cap_setxattr_convert_nscap is called.
I have the untested version on my for-testing branch and except for
a small increase in the binary size of the kernel all seems well.
Eric
next prev parent reply other threads:[~2017-04-23 1:14 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
[not found] ` <87a879sarn.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2017-04-21 21:50 ` Serge E. Hallyn
2017-04-21 21:53 ` Eric W. Biederman
[not found] ` <20170419164824.GA27843-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org>
2017-04-21 23:58 ` Eric W. Biederman
[not found] ` <87wpadpb3m.fsf-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
2017-04-22 15:14 ` Serge E. Hallyn
[not found] ` <20170422151412.GA14861-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org>
2017-04-23 1:14 ` Eric W. Biederman [this message]
2017-04-27 16:20 ` Eric W. Biederman
2017-04-27 16:52 ` Serge E. Hallyn
[not found] ` <20170427165245.GA794-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org>
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=87vapwncws.fsf@xmission.com \
--to=ebiederm-as9lmozglivwk0htik3j/w@public.gmane.org \
--cc=agruenba-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \
--cc=keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
--cc=linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
--cc=luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=morgan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
--cc=serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org \
--cc=seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org \
/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).