Message-ID: <7711bdba-9fbd-406c-8b81-adf91074d0b7@huaweicloud.com>
Date: Sat, 20 Jul 2024 17:31:01 +0800
From: Xu Kuohai <xukuohai@...weicloud.com>
To: Paul Moore <paul@...l-moore.com>, bpf@...r.kernel.org,
 netdev@...r.kernel.org, linux-security-module@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-integrity@...r.kernel.org,
 selinux@...r.kernel.org
Cc: Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>,
 Martin KaFai Lau <martin.lau@...ux.dev>, Eduard Zingerman
 <eddyz87@...il.com>, Song Liu <song@...nel.org>,
 Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
 Matt Bobrowski <mattbobrowski@...gle.com>,
 Brendan Jackman <jackmanb@...omium.org>, James Morris <jmorris@...ei.org>,
 "Serge E . Hallyn" <serge@...lyn.com>,
 Khadija Kamran <kamrankhadijadj@...il.com>,
 Casey Schaufler <casey@...aufler-ca.com>,
 Ondrej Mosnacek <omosnace@...hat.com>, Kees Cook <keescook@...omium.org>,
 John Johansen <john.johansen@...onical.com>,
 Lukas Bulwahn <lukas.bulwahn@...il.com>,
 Roberto Sassu <roberto.sassu@...wei.com>,
 Shung-Hsi Yu <shung-hsi.yu@...e.com>, Edward Cree <ecree.xilinx@...il.com>,
 Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>,
 Trond Myklebust <trond.myklebust@...merspace.com>,
 Anna Schumaker <anna@...nel.org>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Stephen Smalley <stephen.smalley.work@...il.com>
Subject: Re: [PATCH v4 9/20] lsm: Refactor return value of LSM hook
 key_getsecurity

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@...weicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook key_getsecurity to 0 or a negative error code.
>>
>> Before:
>> - Hook key_getsecurity returns length of value on success or a
>>    negative error code on failure.
>>
>> After:
>> - Hook key_getsecurity returns 0 on success or a negative error
>>    code on failure. An output parameter @len is introduced to hold
>>    the length of value on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@...wei.com>
>> ---
>>   include/linux/lsm_hook_defs.h |  3 ++-
>>   include/linux/security.h      |  6 ++++--
>>   security/keys/keyctl.c        | 11 ++++++++---
>>   security/security.c           | 26 +++++++++++++++++++++-----
>>   security/selinux/hooks.c      | 11 +++++------
>>   security/smack/smack_lsm.c    | 21 +++++++++++----------
>>   6 files changed, 51 insertions(+), 27 deletions(-)
> 
> ...
> 
>> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
>> index 4bc3e9398ee3..e9f857620f28 100644
>> --- a/security/keys/keyctl.c
>> +++ b/security/keys/keyctl.c
>> @@ -1565,6 +1565,7 @@ long keyctl_get_security(key_serial_t keyid,
>>   	struct key *key, *instkey;
>>   	key_ref_t key_ref;
>>   	char *context;
>> +	size_t len;
>>   	long ret;
>>   
>>   	key_ref = lookup_user_key(keyid, KEY_LOOKUP_PARTIAL, KEY_NEED_VIEW);
>> @@ -1586,15 +1587,18 @@ long keyctl_get_security(key_serial_t keyid,
>>   	}
>>   
>>   	key = key_ref_to_ptr(key_ref);
>> -	ret = security_key_getsecurity(key, &context);
>> -	if (ret == 0) {
>> +	ret = security_key_getsecurity(key, &context, &len);
>> +	if (ret < 0)
>> +		goto error;
> 
> Since there is already an if-else pattern here you might as well stick
> with that, for example:
> 
>    if (ret == 0) {
>      ...
>    } else if (ret > 0) {
>      ...
>    }
> 
> ... you should probably add a comment that @ret is -ERRNO on failure,
> but it doesn't look like you need an explicit test here since the error
> case will normally fall through to the 'error' label (which you shouldn't
> need anymore either).
>

Got it, thanks for pointing that out.

>> +	if (len == 0) {
>>   		/* if no information was returned, give userspace an empty
>>   		 * string */
>>   		ret = 1;
>>   		if (buffer && buflen > 0 &&
>>   		    copy_to_user(buffer, "", 1) != 0)
>>   			ret = -EFAULT;
>> -	} else if (ret > 0) {
>> +	} else {
>> +		ret = len;
>>   		/* return as much data as there's room for */
>>   		if (buffer && buflen > 0) {
>>   			if (buflen > ret)
>> @@ -1607,6 +1611,7 @@ long keyctl_get_security(key_serial_t keyid,
>>   		kfree(context);
>>   	}
>>   
>> +error:
>>   	key_ref_put(key_ref);
>>   	return ret;
>>   }
>> diff --git a/security/security.c b/security/security.c
>> index 9dd2ae6cf763..2c161101074d 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -5338,19 +5338,35 @@ int security_key_permission(key_ref_t key_ref, const struct cred *cred,
>>    * security_key_getsecurity() - Get the key's security label
>>    * @key: key
>>    * @buffer: security label buffer
>> + * @len: the length of @buffer (including terminating NULL) on success
>>    *
>>    * Get a textual representation of the security context attached to a key for
>>    * the purposes of honouring KEYCTL_GETSECURITY.  This function allocates the
>>    * storage for the NUL-terminated string and the caller should free it.
>>    *
>> - * Return: Returns the length of @buffer (including terminating NUL) or -ve if
>> - *         an error occurs.  May also return 0 (and a NULL buffer pointer) if
>> - *         there is no security label assigned to the key.
>> + * Return: Returns 0 on success or -ve if an error occurs. May also return 0
>> + *         (and a NULL buffer pointer) if there is no security label assigned
>> + *         to the key.
>>    */
>> -int security_key_getsecurity(struct key *key, char **buffer)
>> +int security_key_getsecurity(struct key *key, char **buffer, size_t *len)
>>   {
>> +	int rc;
>> +	size_t n = 0;
>> +	struct security_hook_list *hp;
>> +
>>   	*buffer = NULL;
>> -	return call_int_hook(key_getsecurity, key, buffer);
>> +
>> +	hlist_for_each_entry(hp, &security_hook_heads.key_getsecurity, list) {
>> +		rc = hp->hook.key_getsecurity(key, buffer, &n);
>> +		if (rc < 0)
>> +			return rc;
>> +		if (n)
>> +			break;
>> +	}
>> +
>> +	*len = n;
>> +
>> +	return 0;
>>   }
> 
> Help me understand why we can't continue to use the call_int_hook()
> macro here?
>

Before this patch, the hook may return +ve, 0, or -ve, and call_int_hook
breaks the loop when the hook return value is not 0.

After this patch, the +ve is stored in @n, so @n and return value should
both be checked to determine whether to break the loop. This is not
feasible with call_int_hook.

>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 16cd336aab3d..747ec602dec0 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6737,18 +6737,17 @@ static int selinux_key_permission(key_ref_t key_ref,
>>   	return avc_has_perm(sid, ksec->sid, SECCLASS_KEY, perm, NULL);
>>   }
>>   
>> -static int selinux_key_getsecurity(struct key *key, char **_buffer)
>> +static int selinux_key_getsecurity(struct key *key, char **_buffer,
>> +				   size_t *_len)
>>   {
>>   	struct key_security_struct *ksec = key->security;
>>   	char *context = NULL;
>> -	unsigned len;
>> +	u32 context_len;
> 
> Since @len doesn't collide with the parameter, you might as well just
> stick with @len as the local variable name.
>

Got it

>>   	int rc;
>>   
>> -	rc = security_sid_to_context(ksec->sid,
>> -				     &context, &len);
>> -	if (!rc)
>> -		rc = len;
>> +	rc = security_sid_to_context(ksec->sid, &context, &context_len);
>>   	*_buffer = context;
>> +	*_len = context_len;
>>   	return rc;
>>   }
> 
> --
> paul-moore.com
> 

Powered by blists - more mailing lists