From: "J. Bruce Fields" <[email protected]>
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 <[email protected]>
---
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
Quoting J. Bruce Fields ([email protected]):
> From: "J. Bruce Fields" <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
> ---
> 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
>
From: "J. Bruce Fields" <[email protected]>
Make it clear that cap_inode_getsecctx shouldn't return success without
filling in the context data.
Acked-by: Serge E. Hallyn <[email protected]>
Signed-off-by: J. Bruce Fields <[email protected]>
---
include/linux/security.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/security.h b/include/linux/security.h
index 4686491..40560f4 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1392,7 +1392,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, returns 0 and 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.
--
1.7.9.5
On Thu, 9 May 2013, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <[email protected]>
>
> Make it clear that cap_inode_getsecctx shouldn't return success without
> filling in the context data.
>
> Acked-by: Serge E. Hallyn <[email protected]>
> Signed-off-by: J. Bruce Fields <[email protected]>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next
--
James Morris
<[email protected]>
Quoting J. Bruce Fields ([email protected]):
> On Thu, May 09, 2013 at 12:49:26AM -0500, Serge Hallyn wrote:
> > Quoting J. Bruce Fields ([email protected]):
> > > From: "J. Bruce Fields" <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > 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 <[email protected]>
>
> 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 <[email protected]>
> *
> * @inode we wish to get the security context of.
> * @ctx is a pointer in which to place the allocated security context.
>
> --b.
On Thu, May 09, 2013 at 12:49:26AM -0500, Serge Hallyn wrote:
> Quoting J. Bruce Fields ([email protected]):
> > From: "J. Bruce Fields" <[email protected]>
> >
> > 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 <[email protected]>
>
> 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 <[email protected]>
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.