[PATCH] lsm: fix default return value of the socket_getpeersec_* hooks

[Casey Schaufler]

On 1/29/2024 3:02 PM, Paul Moore wrote:
> On Fri, Jan 26, 2024 at 1:45 PM Ondrej Mosnacek <omosnace at redhat.com> wrote:
>> For these hooks the true "neutral" value is -EOPNOTSUPP, which is
>> currently what is returned when no LSM provides this hook and what LSMs
>> return when there is no security context set on the socket. Correct the
>> value in <linux/lsm_hooks.h> and adjust the dispatch functions in
>> security/security.c to avoid issues when the BPF LSM is enabled.
>>
>> Fixes: 98e828a0650f ("security: Refactor declaration of LSM hooks")
>> Signed-off-by: Ondrej Mosnacek <omosnace at redhat.com>
>> ---
>>  include/linux/lsm_hook_defs.h |  4 ++--
>>  security/security.c           | 31 +++++++++++++++++++++++++++----
>>  2 files changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index 185924c56378..76458b6d53da 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -315,9 +315,9 @@ LSM_HOOK(int, 0, socket_getsockopt, struct socket *sock, int level, int optname)
>>  LSM_HOOK(int, 0, socket_setsockopt, struct socket *sock, int level, int optname)
>>  LSM_HOOK(int, 0, socket_shutdown, struct socket *sock, int how)
>>  LSM_HOOK(int, 0, socket_sock_rcv_skb, struct sock *sk, struct sk_buff *skb)
>> -LSM_HOOK(int, 0, socket_getpeersec_stream, struct socket *sock,
>> +LSM_HOOK(int, -ENOPROTOOPT, socket_getpeersec_stream, struct socket *sock,
>>          sockptr_t optval, sockptr_t optlen, unsigned int len)
>> -LSM_HOOK(int, 0, socket_getpeersec_dgram, struct socket *sock,
>> +LSM_HOOK(int, -ENOPROTOOPT, socket_getpeersec_dgram, struct socket *sock,
>>          struct sk_buff *skb, u32 *secid)
>>  LSM_HOOK(int, 0, sk_alloc_security, struct sock *sk, int family, gfp_t priority)
>>  LSM_HOOK(void, LSM_RET_VOID, sk_free_security, struct sock *sk)
>> diff --git a/security/security.c b/security/security.c
>> index 6196ccaba433..3aaad75c9ce8 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -4624,8 +4624,20 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
>>  int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
>>                                       sockptr_t optlen, unsigned int len)
>>  {
>> -       return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
>> -                            optval, optlen, len);
>> +       struct security_hook_list *hp;
>> +       int rc;
>> +
>> +       /*
>> +        * Only one module will provide a security context.
>> +        */
>> +       hlist_for_each_entry(hp, &security_hook_heads.socket_getpeersec_stream,
>> +                            list) {
>> +               rc = hp->hook.socket_getpeersec_stream(sock, optval, optlen,
>> +                                                      len);
>> +               if (rc != LSM_RET_DEFAULT(socket_getpeersec_stream))
>> +                       return rc;
>> +       }
>> +       return LSM_RET_DEFAULT(socket_getpeersec_stream);
>>  }
> I'm beginning to wonder if we shouldn't update call_int_hook() so that
> it works for LSM_RET_DEFAULT() instead of assuming a zero/0 return
> value.  Thoughts?

call_int_hook() was intended to address the "normal" case, where all
hooks registered would be called and the first error, if any, would
result in an immediate failure return. Hooks that behaved in any other
manner were expected to be open coded. The point of using the macros
was to reduce so much code duplication. I really don't want to see
call_int_hook() evolve into something hard to work with, or that has
non-obvious side effects. I think we could probably integrate
LSM_RET_DEFAULT() safely, but I'm wary of hiding these abnormal cases
in the macro.