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

[Xu Kuohai]

On 7/23/2024 5:35 AM, Paul Moore wrote:
> On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <xukuohai at huaweicloud.com> wrote:
>> On 7/19/2024 10:08 AM, Paul Moore wrote:
>>> On Jul 11, 2024 Xu Kuohai <xukuohai at 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 at 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/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.
> 
> Yes, gotcha.  I was focused on the error condition and wasn't thinking
> about the length getting zero'd out by a trailing callback.
> Unfortunately, we *really* want to stick with the
> call_{int,void}_hook() macros so I think we either need to find a way
> to work within that constraint for existing macro callers, or we have
> to leave this hook as-is for the moment.
> 

Let's leave it as is. So we ultimately have four hooks that can be
converted, two of which require adding additional output parameter to
hold odd return values. These output parameters require extra work
on the BPF verifier, and it doesn't seem worthwhile just for two
hooks. So I prefer to keep only the two patches that handle
conversion without adding output parameters (patch 1 and 5).