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

[Casey Schaufler]

On 1/30/2024 12:29 AM, Ondrej Mosnacek wrote:
> On Tue, Jan 30, 2024 at 4:01 AM Paul Moore <paul at paul-moore.com> wrote:
>> On Mon, Jan 29, 2024 at 6:25 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
>>> 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.
>> Yes, I'm not talking about modifying call_int_hook() to handle
>> something like security_vm_enough_memory_mm(), I'm just talking about
>> updating it use LSM_RET_DEFAULT() instead of zero.
>>
>> While we are at it, we should probably get rid of the second parameter
>> too, @IRC, and just use the assigned LSM_RET_DEFAULT().  That always
>> struck me as a bug waiting to happen if/when those two fell out of
>> sync.
> You're reading my mind :) I already started writing a patch that does
> exactly that after I posted the security_inode_getsecctx() patch.
> While working on it I gradually found two more pre-existing issues, so
> I wanted to post fixes for them before the refactor for better
> backportability. I should be able to post the patch today.
>
> BTW, the IRC param removal means that a few of the existing
> call_int_hook() calls have to be open-coded, but even then the patch
> removes more LoC than it adds, so I think it's worth it.

OK, I'm good with it. Getting rid of @IRC is the clincher.