From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) Subject: Re: [PATCH v4] Introduce v3 namespaced file capabilities Date: Tue, 09 May 2017 17:27:03 -0500 Message-ID: <87mvalu0nc.fsf@xmission.com> References: <20170507092105.GA67584@inn.lkp.intel.com> <20170508044408.GA11400@mail.hallyn.com> <20170508181156.GA23112@mail.hallyn.com> <87a86mvuko.fsf@xmission.com> <20170509203736.GB14900@mail.hallyn.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170509203736.GB14900-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org> (Serge E. Hallyn's message of "Tue, 9 May 2017 15:37:36 -0500") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: "Serge E. Hallyn" Cc: containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, lkp-JC7UmRfGjtg@public.gmane.org, xiaolong.ye-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, LKML List-Id: containers.vger.kernel.org "Serge E. Hallyn" writes: > Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org): >> "Serge E. Hallyn" writes: >> > Changelog: >> [snip] >> > May 8, 2017: >> > . fix leaking dentry refcount in cap_inode_getsecurity >> > >> [snip] >> > +/* >> > + * getsecurity: We are called for security.* before any attempt to read the >> > + * xattr from the inode itself. >> > + * >> > + * This gives us a chance to read the on-disk value and convert it. If we >> > + * return -EOPNOTSUPP, then vfs_getxattr() will call the i_op handler. >> > + * >> > + * Note we are not called by vfs_getxattr_alloc(), but that is only called >> > + * by the integrity subsystem, which really wants the unconverted values - >> > + * so that's good. >> > + */ >> > +int cap_inode_getsecurity(struct inode *inode, const char *name, void **buffer, >> > + bool alloc) >> > +{ >> > + int size, ret; >> > + kuid_t kroot; >> > + uid_t root, mappedroot; >> > + char *tmpbuf = NULL; >> > + struct vfs_cap_data *cap; >> > + struct vfs_ns_cap_data *nscap; >> > + struct dentry *dentry; >> > + struct user_namespace *fs_ns; >> > + >> > + if (strcmp(name, "capability") != 0) >> > + return -EOPNOTSUPP; >> > + >> > + dentry = d_find_alias(inode); >> > + if (!dentry) >> > + return -EINVAL; >> > + >> > + size = sizeof(struct vfs_ns_cap_data); >> > + ret = (int) vfs_getxattr_alloc(dentry, XATTR_NAME_CAPS, >> > + &tmpbuf, size, GFP_NOFS); >> > + dput(dentry); >> >> This looks like a good fix but ouch! That interface is wrong. >> >> The dentry is needed because vfs_getxattr_alloc does: >> error = handler->get(handler, dentry, inode, name, NULL, 0); >> >> Which is has no business taking a dentry as xattrs are inode concepts. >> >> I have no issue with your patch but it looks like that handler issue >> is going to need to be fixed with xattrs. > > True, it's a bit clunky. > > Any reason not to just have the current vfs_getxattr_alloc() become a > lightweight wrapper calling inode_getxattr_alloc(dentry->d_inode)? My deep issue is that handler is functions like posix_acl_xattr_get. And all of those functions that vfs_getxattr_alloc calls should not take a dentry. So I feel like I have just spotted the tip of an iceberg that needs sorting out. Eric From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750926AbdEIWde (ORCPT ); Tue, 9 May 2017 18:33:34 -0400 Received: from out02.mta.xmission.com ([166.70.13.232]:45209 "EHLO out02.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750751AbdEIWdc (ORCPT ); Tue, 9 May 2017 18:33:32 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: "Serge E. Hallyn" Cc: Masami Ichikawa , containers@lists.linux-foundation.org, lkp@01.org, xiaolong.ye@intel.com, LKML References: <20170507092105.GA67584@inn.lkp.intel.com> <20170508044408.GA11400@mail.hallyn.com> <20170508181156.GA23112@mail.hallyn.com> <87a86mvuko.fsf@xmission.com> <20170509203736.GB14900@mail.hallyn.com> Date: Tue, 09 May 2017 17:27:03 -0500 In-Reply-To: <20170509203736.GB14900@mail.hallyn.com> (Serge E. Hallyn's message of "Tue, 9 May 2017 15:37:36 -0500") Message-ID: <87mvalu0nc.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=1d8Dh5-0000fA-8S;;;mid=<87mvalu0nc.fsf@xmission.com>;;;hst=in01.mta.xmission.com;;;ip=97.121.81.159;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX1+jsk/sqwFG/yBFS/YBuJijAZ4LOvf0Kkk= X-SA-Exim-Connect-IP: 97.121.81.159 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.4992] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa01 1397; Body=1 Fuz1=1 Fuz2=1] * 0.1 XMSolicitRefs_0 Weightloss drug X-Spam-DCC: XMission; sa01 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;"Serge E. Hallyn" X-Spam-Relay-Country: X-Spam-Timing: total 5702 ms - load_scoreonly_sql: 0.06 (0.0%), signal_user_changed: 3.0 (0.1%), b_tie_ro: 1.98 (0.0%), parse: 1.37 (0.0%), extract_message_metadata: 32 (0.6%), get_uri_detail_list: 3.0 (0.1%), tests_pri_-1000: 12 (0.2%), tests_pri_-950: 2.0 (0.0%), tests_pri_-900: 1.63 (0.0%), tests_pri_-400: 32 (0.6%), check_bayes: 30 (0.5%), b_tokenize: 12 (0.2%), b_tok_get_all: 8 (0.1%), b_comp_prob: 4.4 (0.1%), b_tok_touch_all: 2.8 (0.0%), b_finish: 0.82 (0.0%), tests_pri_0: 460 (8.1%), check_dkim_signature: 0.96 (0.0%), check_dkim_adsp: 4.6 (0.1%), tests_pri_500: 5152 (90.4%), poll_dns_idle: 5143 (90.2%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH v4] 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 in01.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: >> > Changelog: >> [snip] >> > May 8, 2017: >> > . fix leaking dentry refcount in cap_inode_getsecurity >> > >> [snip] >> > +/* >> > + * getsecurity: We are called for security.* before any attempt to read the >> > + * xattr from the inode itself. >> > + * >> > + * This gives us a chance to read the on-disk value and convert it. If we >> > + * return -EOPNOTSUPP, then vfs_getxattr() will call the i_op handler. >> > + * >> > + * Note we are not called by vfs_getxattr_alloc(), but that is only called >> > + * by the integrity subsystem, which really wants the unconverted values - >> > + * so that's good. >> > + */ >> > +int cap_inode_getsecurity(struct inode *inode, const char *name, void **buffer, >> > + bool alloc) >> > +{ >> > + int size, ret; >> > + kuid_t kroot; >> > + uid_t root, mappedroot; >> > + char *tmpbuf = NULL; >> > + struct vfs_cap_data *cap; >> > + struct vfs_ns_cap_data *nscap; >> > + struct dentry *dentry; >> > + struct user_namespace *fs_ns; >> > + >> > + if (strcmp(name, "capability") != 0) >> > + return -EOPNOTSUPP; >> > + >> > + dentry = d_find_alias(inode); >> > + if (!dentry) >> > + return -EINVAL; >> > + >> > + size = sizeof(struct vfs_ns_cap_data); >> > + ret = (int) vfs_getxattr_alloc(dentry, XATTR_NAME_CAPS, >> > + &tmpbuf, size, GFP_NOFS); >> > + dput(dentry); >> >> This looks like a good fix but ouch! That interface is wrong. >> >> The dentry is needed because vfs_getxattr_alloc does: >> error = handler->get(handler, dentry, inode, name, NULL, 0); >> >> Which is has no business taking a dentry as xattrs are inode concepts. >> >> I have no issue with your patch but it looks like that handler issue >> is going to need to be fixed with xattrs. > > True, it's a bit clunky. > > Any reason not to just have the current vfs_getxattr_alloc() become a > lightweight wrapper calling inode_getxattr_alloc(dentry->d_inode)? My deep issue is that handler is functions like posix_acl_xattr_get. And all of those functions that vfs_getxattr_alloc calls should not take a dentry. So I feel like I have just spotted the tip of an iceberg that needs sorting out. Eric From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============7294464547543548389==" MIME-Version: 1.0 From: Eric W. Biederman To: lkp@lists.01.org Subject: Re: [PATCH v4] Introduce v3 namespaced file capabilities Date: Tue, 09 May 2017 17:27:03 -0500 Message-ID: <87mvalu0nc.fsf@xmission.com> In-Reply-To: <20170509203736.GB14900@mail.hallyn.com> List-Id: --===============7294464547543548389== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable "Serge E. Hallyn" writes: > Quoting Eric W. Biederman (ebiederm(a)xmission.com): >> "Serge E. Hallyn" writes: >> > Changelog: >> [snip] >> > May 8, 2017: >> > . fix leaking dentry refcount in cap_inode_getsecurity >> > >> [snip] >> > +/* >> > + * getsecurity: We are called for security.* before any attempt to re= ad the >> > + * xattr from the inode itself. >> > + * >> > + * This gives us a chance to read the on-disk value and convert it. = If we >> > + * return -EOPNOTSUPP, then vfs_getxattr() will call the i_op handler. >> > + * >> > + * Note we are not called by vfs_getxattr_alloc(), but that is only c= alled >> > + * by the integrity subsystem, which really wants the unconverted val= ues - >> > + * so that's good. >> > + */ >> > +int cap_inode_getsecurity(struct inode *inode, const char *name, void= **buffer, >> > + bool alloc) >> > +{ >> > + int size, ret; >> > + kuid_t kroot; >> > + uid_t root, mappedroot; >> > + char *tmpbuf =3D NULL; >> > + struct vfs_cap_data *cap; >> > + struct vfs_ns_cap_data *nscap; >> > + struct dentry *dentry; >> > + struct user_namespace *fs_ns; >> > + >> > + if (strcmp(name, "capability") !=3D 0) >> > + return -EOPNOTSUPP; >> > + >> > + dentry =3D d_find_alias(inode); >> > + if (!dentry) >> > + return -EINVAL; >> > + >> > + size =3D sizeof(struct vfs_ns_cap_data); >> > + ret =3D (int) vfs_getxattr_alloc(dentry, XATTR_NAME_CAPS, >> > + &tmpbuf, size, GFP_NOFS); >> > + dput(dentry); >> = >> This looks like a good fix but ouch! That interface is wrong. >> = >> The dentry is needed because vfs_getxattr_alloc does: >> error =3D handler->get(handler, dentry, inode, name, NULL, 0); >> = >> Which is has no business taking a dentry as xattrs are inode concepts. >> = >> I have no issue with your patch but it looks like that handler issue >> is going to need to be fixed with xattrs. > > True, it's a bit clunky. > > Any reason not to just have the current vfs_getxattr_alloc() become a > lightweight wrapper calling inode_getxattr_alloc(dentry->d_inode)? My deep issue is that handler is functions like posix_acl_xattr_get. And all of those functions that vfs_getxattr_alloc calls should not take a dentry. So I feel like I have just spotted the tip of an iceberg that needs sorting out. Eric --===============7294464547543548389==--