Re: [PATCH] fix security_release_secctx seems broken

[Konstantin Khlebnikov]

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@xxxxxxxxxxxxxxxx>

Looks good for me.

Reviewed-by: Konstantin Khlebnikov <khlebnikov@xxxxxxxxxxxxxx>


> ---
>   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);
>   }
>   
>