From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1427419AbdDWBUp (ORCPT ); Sat, 22 Apr 2017 21:20:45 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:53958 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1427376AbdDWBUh (ORCPT ); Sat, 22 Apr 2017 21:20:37 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: "Serge E. Hallyn" Cc: Seth Forshee , lkml , linux-api@vger.kernel.org, linux-security-module@vger.kernel.org, Kees Cook , Andreas Gruenbacher , Andy Lutomirski , "Andrew G. Morgan" References: <20170419164824.GA27843@mail.hallyn.com> <87wpadpb3m.fsf@xmission.com> <20170422151412.GA14861@mail.hallyn.com> Date: Sat, 22 Apr 2017 20:14:11 -0500 In-Reply-To: <20170422151412.GA14861@mail.hallyn.com> (Serge E. Hallyn's message of "Sat, 22 Apr 2017 10:14:12 -0500") Message-ID: <87vapwncws.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1d26CG-0005rM-62;;;mid=<87vapwncws.fsf@xmission.com>;;;hst=in02.mta.xmission.com;;;ip=67.3.233.227;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1/ZwoeqNSVhNVxJ3OOe2fcFEtCiqtlv5Q4= X-SA-Exim-Connect-IP: 67.3.233.227 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa05 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa05 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;"Serge E. Hallyn" X-Spam-Relay-Country: X-Spam-Timing: total 13803 ms - load_scoreonly_sql: 0.04 (0.0%), signal_user_changed: 2.5 (0.0%), b_tie_ro: 1.75 (0.0%), parse: 0.79 (0.0%), extract_message_metadata: 13 (0.1%), get_uri_detail_list: 1.78 (0.0%), tests_pri_-1000: 4.7 (0.0%), tests_pri_-950: 1.13 (0.0%), tests_pri_-900: 0.94 (0.0%), tests_pri_-400: 23 (0.2%), check_bayes: 22 (0.2%), b_tokenize: 7 (0.1%), b_tok_get_all: 7 (0.1%), b_comp_prob: 2.3 (0.0%), b_tok_touch_all: 3.4 (0.0%), b_finish: 0.53 (0.0%), tests_pri_0: 250 (1.8%), check_dkim_signature: 0.50 (0.0%), check_dkim_adsp: 2.9 (0.0%), tests_pri_500: 13504 (97.8%), poll_dns_idle: 13492 (97.7%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH] Introduce v3 namespaced file capabilities X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -0600) X-SA-Exim-Scanned: Yes (on in02.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org "Serge E. Hallyn" writes: > Quoting Eric W. Biederman (ebiederm@xmission.com): >> >> "Serge E. Hallyn" 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