[bpf-next,v4,09/20] lsm: Refactor return value of LSM hook key_getsecurity

Commit Message

Xu Kuohai July 11, 2024, 11:18 a.m. UTC

From: Xu Kuohai <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Paul Moore July 19, 2024, 2:08 a.m. UTC | #1

On Jul 11, 2024 Xu Kuohai <[e-mail]> 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 <[e-mail]>
> ---
>  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).

> +	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?

> 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.

>  	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

Xu Kuohai July 20, 2024, 9:31 a.m. UTC | #2

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <[e-mail]> 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 <[e-mail]>
>> ---
>>   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
>

Paul Moore July 22, 2024, 9:35 p.m. UTC | #3

On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <[e-mail]> wrote:
> On 7/19/2024 10:08 AM, Paul Moore wrote:
> > On Jul 11, 2024 Xu Kuohai <[e-mail]> 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 <[e-mail]>
> >> ---
> >>   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.

Xu Kuohai July 23, 2024, 7:04 a.m. UTC | #4

On 7/23/2024 5:35 AM, Paul Moore wrote:
> On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <[e-mail]> wrote:
>> On 7/19/2024 10:08 AM, Paul Moore wrote:
>>> On Jul 11, 2024 Xu Kuohai <[e-mail]> 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 <[e-mail]>
>>>> ---
>>>>    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).

Paul Moore July 23, 2024, 6:34 p.m. UTC | #5

On Tue, Jul 23, 2024 at 3:04 AM Xu Kuohai <[e-mail]> wrote:
> On 7/23/2024 5:35 AM, Paul Moore wrote:
> > On Sat, Jul 20, 2024 at 5:31 AM Xu Kuohai <[e-mail]> wrote:
> >> On 7/19/2024 10:08 AM, Paul Moore wrote:
> >>> On Jul 11, 2024 Xu Kuohai <[e-mail]> 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 <[e-mail]>
> >>>> ---
> >>>>    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).

Fair enough.  Thanks for working on this, between the changes to the
LSM framework and the BPF verifier, I think this is still a nice
improvement.

Patch

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index b0e3cf3fc33f..54fec360947c 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -407,7 +407,8 @@  LSM_HOOK(int, 0, key_alloc, struct key *key, const struct cred *cred,
 LSM_HOOK(void, LSM_RET_VOID, key_free, struct key *key)
 LSM_HOOK(int, 0, key_permission, key_ref_t key_ref, const struct cred *cred,
 	 enum key_need_perm need_perm)
-LSM_HOOK(int, 0, key_getsecurity, struct key *key, char **buffer)
+LSM_HOOK(int, 0, key_getsecurity, struct key *key, char **buffer,
+	 size_t *len)
 LSM_HOOK(void, LSM_RET_VOID, key_post_create_or_update, struct key *keyring,
 	 struct key *key, const void *payload, size_t payload_len,
 	 unsigned long flags, bool create)
diff --git a/include/linux/security.h b/include/linux/security.h
index 616047030a89..4e64e51a1a86 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2020,7 +2020,7 @@  int security_key_alloc(struct key *key, const struct cred *cred, unsigned long f
 void security_key_free(struct key *key);
 int security_key_permission(key_ref_t key_ref, const struct cred *cred,
 			    enum key_need_perm need_perm);
-int security_key_getsecurity(struct key *key, char **_buffer);
+int security_key_getsecurity(struct key *key, char **_buffer, size_t *_len);
 void security_key_post_create_or_update(struct key *keyring, struct key *key,
 					const void *payload, size_t payload_len,
 					unsigned long flags, bool create);
@@ -2045,9 +2045,11 @@  static inline int security_key_permission(key_ref_t key_ref,
 	return 0;
 }
 
-static inline int security_key_getsecurity(struct key *key, char **_buffer)
+static inline int security_key_getsecurity(struct key *key, char **_buffer,
+					   size_t *_len)
 {
 	*_buffer = NULL;
+	*_len = 0;
 	return 0;
 }
 
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;
+	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;
 }
 
 /**
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;
 	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;
 }
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 8a352bd05565..9a121ad53b16 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4585,30 +4585,31 @@  static int smack_key_permission(key_ref_t key_ref,
 /*
  * smack_key_getsecurity - Smack label tagging the key
  * @key points to the key to be queried
- * @_buffer points to a pointer that should be set to point to the
- * resulting string (if no label or an error occurs).
- * Return the length of the string (including terminating NUL) or -ve if
- * an error.
- * May also return 0 (and a NULL buffer pointer) if there is no label.
+ * @_buffer points to a pointer that should be set to point to the resulting
+ *          string (if no label or an error occurs).
+ * @_len  the length of the @_buffer (including terminating NUL)
+ *
+ * Return 0 on success or -ve if an error.
+ * If there is no label, @_buffer will be set to NULL and @_len will be set to
+ * 0.
  */
-static int smack_key_getsecurity(struct key *key, char **_buffer)
+static int smack_key_getsecurity(struct key *key, char **_buffer, size_t *_len)
 {
 	struct smack_known *skp = key->security;
-	size_t length;
 	char *copy;
 
 	if (key->security == NULL) {
 		*_buffer = NULL;
+		*_len = 0;
 		return 0;
 	}
 
 	copy = kstrdup(skp->smk_known, GFP_KERNEL);
 	if (copy == NULL)
 		return -ENOMEM;
-	length = strlen(copy) + 1;
-
+	*_len = strlen(copy) + 1;
 	*_buffer = copy;
-	return length;
+	return 0;
 }