Re: [PATCH v4 9/20] lsm: Refactor return value of LSM hook key_getsecurity

Xu Kuohai
2024-07-20T09:31:05Z

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.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@huawei.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
>