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

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

[Ondrej Mosnacek]

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@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);
 }
 
 /**
@@ -4645,8 +4657,19 @@ int security_socket_getpeersec_stream(struct socket *sock, sockptr_t optval,
 int security_socket_getpeersec_dgram(struct socket *sock,
 				     struct sk_buff *skb, u32 *secid)
 {
-	return call_int_hook(socket_getpeersec_dgram, -ENOPROTOOPT, sock,
-			     skb, secid);
+	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_dgram,
+			     list) {
+		rc = hp->hook.socket_getpeersec_dgram(sock, skb, secid);
+		if (rc != LSM_RET_DEFAULT(socket_getpeersec_dgram))
+			return rc;
+	}
+	return LSM_RET_DEFAULT(socket_getpeersec_dgram);
 }
 EXPORT_SYMBOL(security_socket_getpeersec_dgram);
 
-- 
2.43.0

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

[Paul Moore]

On Fri, Jan 26, 2024 at 1:45 PM Ondrej Mosnacek <omosnace@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@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?

-- 
paul-moore.com

Re: [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@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@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.

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

[Paul Moore]

On Mon, Jan 29, 2024 at 6:25 PM Casey Schaufler <casey@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@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@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.

--
paul-moore.com

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

[Ondrej Mosnacek]

On Tue, Jan 30, 2024 at 4:01 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Jan 29, 2024 at 6:25 PM Casey Schaufler <casey@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@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@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.

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [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@paul-moore.com> wrote:
>> On Mon, Jan 29, 2024 at 6:25 PM Casey Schaufler <casey@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@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@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.

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

[Paul Moore]

On Jan 26, 2024 Ondrej Mosnacek <omosnace@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@redhat.com>
> ---
>  include/linux/lsm_hook_defs.h |  4 ++--
>  security/security.c           | 31 +++++++++++++++++++++++++++----
>  2 files changed, 29 insertions(+), 6 deletions(-)

I was originally going to merge this via lsm/dev, but thinking about
this some more today, and considering the other inode_getsecctx() fix,
I think this patch should be marked for stable too.

I'm going to merge this into lsm/stable-6.8 and assuming all the tests
come back clean (which they should), I'll send this up to Linus
tomorrow with the inode_getsecctx() fix.

Thanks all!

--
paul-moore.com