| Message ID | 20130509014329.GH23747@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting J. Bruce Fields (bfields@fieldses.org): > From: "J. Bruce Fields" <bfields@redhat.com> > > We shouldn't be returning success from this function without also > filling in the return values ctx and ctxlen. > > Note currently this doesn't appear to cause bugs since the only > inode_getsecctx caller I can find is fs/sysfs/inode.c, which only calls > this if security_inode_setsecurity succeeds. Assuming > security_inode_setsecurity is set to cap_inode_setsecurity whenever > inode_getsecctx is set to cap_inode_getsecctx, this function can never > actually called. > > So I noticed this only because the server labeled NFS patches add a real > caller. > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> Thanks, the comment in include/linux/security.h doesn't mention the return value at all, but based on the other implementations this looks right. Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com> > --- > security/capability.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > Also, assuming this is correct, would you be willing to apply it for > 3.10? > > If you'd prefer it wait till the next merge window: could you ACK it, > and let me merge it through the nfsd tree? (It's a prerequisite for > the labeled NFS patches that I hope to merge for 3.11.) > > diff --git a/security/capability.c b/security/capability.c > index d32e16e..32b5157 100644 > --- a/security/capability.c > +++ b/security/capability.c > @@ -858,7 +858,7 @@ static int cap_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen) > > static int cap_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen) > { > - return 0; > + return -EOPNOTSUPP; > } > #ifdef CONFIG_KEYS > static int cap_key_alloc(struct key *key, const struct cred *cred, > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 09, 2013 at 12:49:26AM -0500, Serge Hallyn wrote: > Quoting J. Bruce Fields (bfields@fieldses.org): > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > We shouldn't be returning success from this function without also > > filling in the return values ctx and ctxlen. > > > > Note currently this doesn't appear to cause bugs since the only > > inode_getsecctx caller I can find is fs/sysfs/inode.c, which only calls > > this if security_inode_setsecurity succeeds. Assuming > > security_inode_setsecurity is set to cap_inode_setsecurity whenever > > inode_getsecctx is set to cap_inode_getsecctx, this function can never > > actually called. > > > > So I noticed this only because the server labeled NFS patches add a real > > caller. > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > Thanks, the comment in include/linux/security.h doesn't mention the > return value at all, but based on the other implementations this looks > right. > > Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com> Thanks! Hm, would something like this help clarify?: @@ -1412,7 +1412,8 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts) * @ctxlen contains the length of @ctx. * * @inode_getsecctx: - * Returns a string containing all relevant security context information + * On success, fills out @ctx and @ctxlen with the security context + * for the given @inode. * * @inode we wish to get the security context of. * @ctx is a pointer in which to place the allocated security context. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting J. Bruce Fields (bfields@fieldses.org): > On Thu, May 09, 2013 at 12:49:26AM -0500, Serge Hallyn wrote: > > Quoting J. Bruce Fields (bfields@fieldses.org): > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > We shouldn't be returning success from this function without also > > > filling in the return values ctx and ctxlen. > > > > > > Note currently this doesn't appear to cause bugs since the only > > > inode_getsecctx caller I can find is fs/sysfs/inode.c, which only calls > > > this if security_inode_setsecurity succeeds. Assuming > > > security_inode_setsecurity is set to cap_inode_setsecurity whenever > > > inode_getsecctx is set to cap_inode_getsecctx, this function can never > > > actually called. > > > > > > So I noticed this only because the server labeled NFS patches add a real > > > caller. > > > > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > > Thanks, the comment in include/linux/security.h doesn't mention the > > return value at all, but based on the other implementations this looks > > right. > > > > Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com> > > Thanks! Hm, would something like this help clarify?: > > @@ -1412,7 +1412,8 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts) > * @ctxlen contains the length of @ctx. > * > * @inode_getsecctx: > - * Returns a string containing all relevant security context information > + * On success, fills out @ctx and @ctxlen with the security context > + * for the given @inode. ... and returns 0. That would be great, thanks! Acked-by: Serge E. Hallyn <serge.hallyn@ubuntu.com> > * > * @inode we wish to get the security context of. > * @ctx is a pointer in which to place the allocated security context. > > --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/security/capability.c b/security/capability.c index d32e16e..32b5157 100644 --- a/security/capability.c +++ b/security/capability.c @@ -858,7 +858,7 @@ static int cap_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen) static int cap_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen) { - return 0; + return -EOPNOTSUPP; } #ifdef CONFIG_KEYS static int cap_key_alloc(struct key *key, const struct cred *cred,