lkml.org 
[lkml]   [2017]   [Sep]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] fix security_release_secctx seems broken
From
Date
On 19.09.2017 19:39, Casey Schaufler wrote:
> Subject: [PATCH] fix security_release_secctx seems broken
>
> security_inode_getsecurity() provides the text string value
> of a security attribute. It does not provide a "secctx".
> The code in xattr_getsecurity() that calls security_inode_getsecurity()
> and then calls security_release_secctx() happened to work because
> SElinux and Smack treat the attribute and the secctx the same way.
> It fails for cap_inode_getsecurity(), because that module has no
> secctx that ever needs releasing. It turns out that Smack is the
> one that's doing things wrong by not allocating memory when instructed
> to do so by the "alloc" parameter.
>
> The fix is simple enough. Change the security_release_secctx() to
> kfree() because it isn't a secctx being returned by
> security_inode_getsecurity(). Change Smack to allocate the string when
> told to do so.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>

Looks good for me.

Reviewed-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>


>
> ---
> fs/xattr.c | 2 +-
> security/smack/smack_lsm.c | 55 +++++++++++++++++++++-------------------------
> 2 files changed, 26 insertions(+), 31 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 4424f7fecf14..61cd28ba25f3 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -250,7 +250,7 @@ xattr_getsecurity(struct inode *inode, const char *name, void *value,
> }
> memcpy(value, buffer, len);
> out:
> - security_release_secctx(buffer, len);
> + kfree(buffer);
> out_noalloc:
> return len;
> }
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 319add31b4a4..286171a16ed2 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1473,7 +1473,7 @@ static int smack_inode_removexattr(struct dentry *dentry, const char *name)
> * @inode: the object
> * @name: attribute name
> * @buffer: where to put the result
> - * @alloc: unused
> + * @alloc: duplicate memory
> *
> * Returns the size of the attribute or an error code
> */
> @@ -1486,43 +1486,38 @@ static int smack_inode_getsecurity(struct inode *inode,
> struct super_block *sbp;
> struct inode *ip = (struct inode *)inode;
> struct smack_known *isp;
> - int ilen;
> - int rc = 0;
>
> - if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
> + if (strcmp(name, XATTR_SMACK_SUFFIX) == 0)
> isp = smk_of_inode(inode);
> - ilen = strlen(isp->smk_known);
> - *buffer = isp->smk_known;
> - return ilen;
> - }
> + else {
> + /*
> + * The rest of the Smack xattrs are only on sockets.
> + */
> + sbp = ip->i_sb;
> + if (sbp->s_magic != SOCKFS_MAGIC)
> + return -EOPNOTSUPP;
>
> - /*
> - * The rest of the Smack xattrs are only on sockets.
> - */
> - sbp = ip->i_sb;
> - if (sbp->s_magic != SOCKFS_MAGIC)
> - return -EOPNOTSUPP;
> + sock = SOCKET_I(ip);
> + if (sock == NULL || sock->sk == NULL)
> + return -EOPNOTSUPP;
>
> - sock = SOCKET_I(ip);
> - if (sock == NULL || sock->sk == NULL)
> - return -EOPNOTSUPP;
> -
> - ssp = sock->sk->sk_security;
> + ssp = sock->sk->sk_security;
>
> - if (strcmp(name, XATTR_SMACK_IPIN) == 0)
> - isp = ssp->smk_in;
> - else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
> - isp = ssp->smk_out;
> - else
> - return -EOPNOTSUPP;
> + if (strcmp(name, XATTR_SMACK_IPIN) == 0)
> + isp = ssp->smk_in;
> + else if (strcmp(name, XATTR_SMACK_IPOUT) == 0)
> + isp = ssp->smk_out;
> + else
> + return -EOPNOTSUPP;
> + }
>
> - ilen = strlen(isp->smk_known);
> - if (rc == 0) {
> - *buffer = isp->smk_known;
> - rc = ilen;
> + if (alloc) {
> + *buffer = kstrdup(isp->smk_known, GFP_KERNEL);
> + if (*buffer == NULL)
> + return -ENOMEM;
> }
>
> - return rc;
> + return strlen(isp->smk_known);
> }
>
>
>

\
 
 \ /
  Last update: 2017-09-20 13:49    [W:0.078 / U:0.772 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site