On Tue, Jan 30, 2024 at 4:01 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > On Mon, Jan 29, 2024 at 6:25 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: > > > > On 1/29/2024 3:02 PM, Paul Moore wrote: > > > On Fri, Jan 26, 2024 at 1:45 PM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx> > > >> --- > > >> 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. -- Ondrej Mosnacek Senior Software Engineer, Linux Security - SELinux kernel Red Hat, Inc.