[PATCH net] netlabel: fix RCU annotation for IPv4 options on socket creation

[PATCH net] netlabel: fix RCU annotation for IPv4 options on socket creation

[Davide Caratti]

Xiumei reports the following splat when netlabel and TCP socket are used:

 =============================
 WARNING: suspicious RCU usage
 6.9.0-rc2+ #637 Not tainted
 -----------------------------
 net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage!

 other info that might help us debug this:

 rcu_scheduler_active = 2, debug_locks = 1
 1 lock held by ncat/23333:
  #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0

 stack backtrace:
 CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637
 Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0  07/26/2013
 Call Trace:
  <TASK>
  dump_stack_lvl+0xa9/0xc0
  lockdep_rcu_suspicious+0x117/0x190
  cipso_v4_sock_setattr+0x1ab/0x1b0
  netlbl_sock_setattr+0x13e/0x1b0
  selinux_netlbl_socket_post_create+0x3f/0x80
  selinux_socket_post_create+0x1a0/0x460
  security_socket_post_create+0x42/0x60
  __sock_create+0x342/0x3a0
  __sys_socket_create.part.22+0x42/0x70
  __sys_socket+0x37/0xb0
  __x64_sys_socket+0x16/0x20
  do_syscall_64+0x96/0x180
  ? do_user_addr_fault+0x68d/0xa30
  ? exc_page_fault+0x171/0x280
  ? asm_exc_page_fault+0x22/0x30
  entry_SYSCALL_64_after_hwframe+0x71/0x79
 RIP: 0033:0x7fbc0ca3fc1b
 Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48
 RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029
 RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b
 RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002
 RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001
 R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0
 R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000
  </TASK>

The current implementation of cipso_v4_sock_setattr() replaces IP options
under the assumption that the caller holds the socket lock; however, such
assumption is not true, nor needed, in selinux_socket_post_create() hook.

Using rcu_dereference_check() instead of rcu_dereference_protected() will
avoid the reported splat for the netlbl_sock_setattr() case, and preserve
the legitimate check when the caller is netlbl_conn_setattr().

Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt")
Reported-by: Xiumei Mu <xmu@redhat.com>
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
 net/ipv4/cipso_ipv4.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
index 8b17d83e5fde..1d0c2a905078 100644
--- a/net/ipv4/cipso_ipv4.c
+++ b/net/ipv4/cipso_ipv4.c
@@ -1876,8 +1876,10 @@ int cipso_v4_sock_setattr(struct sock *sk,
 
 	sk_inet = inet_sk(sk);
 
-	old = rcu_dereference_protected(sk_inet->inet_opt,
-					lockdep_sock_is_held(sk));
+	/* caller either holds rcu_read_lock() (on socket creation)
+	 * or socket lock (in all other cases). */
+	old = rcu_dereference_check(sk_inet->inet_opt,
+				    lockdep_sock_is_held(sk));
 	if (inet_test_bit(IS_ICSK, sk)) {
 		sk_conn = inet_csk(sk);
 		if (old)
-- 
2.44.0

Re: [PATCH net] netlabel: fix RCU annotation for IPv4 options on socket creation

[Eric Dumazet]

On Thu, Apr 11, 2024 at 5:44 PM Davide Caratti <dcaratti@redhat.com> wrote:
>
> Xiumei reports the following splat when netlabel and TCP socket are used:
>
>  =============================
>  WARNING: suspicious RCU usage
>  6.9.0-rc2+ #637 Not tainted
>  -----------------------------
>  net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage!
>
>  other info that might help us debug this:
>
>  rcu_scheduler_active = 2, debug_locks = 1
>  1 lock held by ncat/23333:
>   #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0
>
>  stack backtrace:
>  CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637
>  Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0  07/26/2013
>  Call Trace:
>   <TASK>
>   dump_stack_lvl+0xa9/0xc0
>   lockdep_rcu_suspicious+0x117/0x190
>   cipso_v4_sock_setattr+0x1ab/0x1b0
>   netlbl_sock_setattr+0x13e/0x1b0
>   selinux_netlbl_socket_post_create+0x3f/0x80
>   selinux_socket_post_create+0x1a0/0x460
>   security_socket_post_create+0x42/0x60
>   __sock_create+0x342/0x3a0
>   __sys_socket_create.part.22+0x42/0x70
>   __sys_socket+0x37/0xb0
>   __x64_sys_socket+0x16/0x20
>   do_syscall_64+0x96/0x180
>   ? do_user_addr_fault+0x68d/0xa30
>   ? exc_page_fault+0x171/0x280
>   ? asm_exc_page_fault+0x22/0x30
>   entry_SYSCALL_64_after_hwframe+0x71/0x79
>  RIP: 0033:0x7fbc0ca3fc1b
>  Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48
>  RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029
>  RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b
>  RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002
>  RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001
>  R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0
>  R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000
>   </TASK>
>
> The current implementation of cipso_v4_sock_setattr() replaces IP options
> under the assumption that the caller holds the socket lock; however, such
> assumption is not true, nor needed, in selinux_socket_post_create() hook.
>
> Using rcu_dereference_check() instead of rcu_dereference_protected() will
> avoid the reported splat for the netlbl_sock_setattr() case, and preserve
> the legitimate check when the caller is netlbl_conn_setattr().
>
> Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt")
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> ---
>  net/ipv4/cipso_ipv4.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 8b17d83e5fde..1d0c2a905078 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -1876,8 +1876,10 @@ int cipso_v4_sock_setattr(struct sock *sk,
>
>         sk_inet = inet_sk(sk);
>
> -       old = rcu_dereference_protected(sk_inet->inet_opt,
> -                                       lockdep_sock_is_held(sk));
> +       /* caller either holds rcu_read_lock() (on socket creation)
> +        * or socket lock (in all other cases). */
> +       old = rcu_dereference_check(sk_inet->inet_opt,
> +                                   lockdep_sock_is_held(sk));
>         if (inet_test_bit(IS_ICSK, sk)) {
>                 sk_conn = inet_csk(sk);
>                 if (old)
> --
> 2.44.0
>

OK, but rcu_read_lock() being held (incidentally by the caller) here
is not protecting the write operation,
so this looks wrong IMO.

Whenever we can not ensure a mutex/spinlock is held, we usually use
rcu_dereference_protected(XXX, 1),
and a comment might simply explain the reason we assert it is protected.

(We also could add a new boolean parameter, set to true or false
depending on the caller)

old = rcu_dereference_protected(sk_inet->inet_opt, from_socket_creation ||

            lockdep_sock_is_held(sk));

Re: [PATCH net] netlabel: fix RCU annotation for IPv4 options on socket creation

[Casey Schaufler]

On 4/11/2024 8:56 AM, Eric Dumazet wrote:
> On Thu, Apr 11, 2024 at 5:44 PM Davide Caratti <dcaratti@redhat.com> wrote:
>> Xiumei reports the following splat when netlabel and TCP socket are used:
>>
>>  =============================
>>  WARNING: suspicious RCU usage
>>  6.9.0-rc2+ #637 Not tainted
>>  -----------------------------
>>  net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage!
>>
>>  other info that might help us debug this:
>>
>>  rcu_scheduler_active = 2, debug_locks = 1
>>  1 lock held by ncat/23333:
>>   #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0
>>
>>  stack backtrace:
>>  CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637
>>  Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0  07/26/2013
>>  Call Trace:
>>   <TASK>
>>   dump_stack_lvl+0xa9/0xc0
>>   lockdep_rcu_suspicious+0x117/0x190
>>   cipso_v4_sock_setattr+0x1ab/0x1b0
>>   netlbl_sock_setattr+0x13e/0x1b0
>>   selinux_netlbl_socket_post_create+0x3f/0x80
>>   selinux_socket_post_create+0x1a0/0x460
>>   security_socket_post_create+0x42/0x60
>>   __sock_create+0x342/0x3a0
>>   __sys_socket_create.part.22+0x42/0x70
>>   __sys_socket+0x37/0xb0
>>   __x64_sys_socket+0x16/0x20
>>   do_syscall_64+0x96/0x180
>>   ? do_user_addr_fault+0x68d/0xa30
>>   ? exc_page_fault+0x171/0x280
>>   ? asm_exc_page_fault+0x22/0x30
>>   entry_SYSCALL_64_after_hwframe+0x71/0x79
>>  RIP: 0033:0x7fbc0ca3fc1b
>>  Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48
>>  RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029
>>  RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b
>>  RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002
>>  RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001
>>  R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0
>>  R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000
>>   </TASK>
>>
>> The current implementation of cipso_v4_sock_setattr() replaces IP options
>> under the assumption that the caller holds the socket lock; however, such
>> assumption is not true, nor needed, in selinux_socket_post_create() hook.
>>
>> Using rcu_dereference_check() instead of rcu_dereference_protected() will
>> avoid the reported splat for the netlbl_sock_setattr() case, and preserve
>> the legitimate check when the caller is netlbl_conn_setattr().
>>
>> Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt")
>> Reported-by: Xiumei Mu <xmu@redhat.com>
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Davide Caratti <dcaratti@redhat.com>

Please be sure to verify that this is appropriate for all users of netlabel.
SELinux is not the only user of netlabel.

>> ---
>>  net/ipv4/cipso_ipv4.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
>> index 8b17d83e5fde..1d0c2a905078 100644
>> --- a/net/ipv4/cipso_ipv4.c
>> +++ b/net/ipv4/cipso_ipv4.c
>> @@ -1876,8 +1876,10 @@ int cipso_v4_sock_setattr(struct sock *sk,
>>
>>         sk_inet = inet_sk(sk);
>>
>> -       old = rcu_dereference_protected(sk_inet->inet_opt,
>> -                                       lockdep_sock_is_held(sk));
>> +       /* caller either holds rcu_read_lock() (on socket creation)
>> +        * or socket lock (in all other cases). */
>> +       old = rcu_dereference_check(sk_inet->inet_opt,
>> +                                   lockdep_sock_is_held(sk));
>>         if (inet_test_bit(IS_ICSK, sk)) {
>>                 sk_conn = inet_csk(sk);
>>                 if (old)
>> --
>> 2.44.0
>>
> OK, but rcu_read_lock() being held (incidentally by the caller) here
> is not protecting the write operation,
> so this looks wrong IMO.
>
> Whenever we can not ensure a mutex/spinlock is held, we usually use
> rcu_dereference_protected(XXX, 1),
> and a comment might simply explain the reason we assert it is protected.
>
> (We also could add a new boolean parameter, set to true or false
> depending on the caller)
>
> old = rcu_dereference_protected(sk_inet->inet_opt, from_socket_creation ||
>
>             lockdep_sock_is_held(sk));
>

Re: [PATCH net] netlabel: fix RCU annotation for IPv4 options on socket creation

[Paul Moore]

On Thu, Apr 11, 2024 at 11:57 AM Eric Dumazet <edumazet@google.com> wrote:
> On Thu, Apr 11, 2024 at 5:44 PM Davide Caratti <dcaratti@redhat.com> wrote:
> >
> > Xiumei reports the following splat when netlabel and TCP socket are used:
> >
> >  =============================
> >  WARNING: suspicious RCU usage
> >  6.9.0-rc2+ #637 Not tainted
> >  -----------------------------
> >  net/ipv4/cipso_ipv4.c:1880 suspicious rcu_dereference_protected() usage!
> >
> >  other info that might help us debug this:
> >
> >  rcu_scheduler_active = 2, debug_locks = 1
> >  1 lock held by ncat/23333:
> >   #0: ffffffff906030c0 (rcu_read_lock){....}-{1:2}, at: netlbl_sock_setattr+0x25/0x1b0
> >
> >  stack backtrace:
> >  CPU: 11 PID: 23333 Comm: ncat Kdump: loaded Not tainted 6.9.0-rc2+ #637
> >  Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0  07/26/2013
> >  Call Trace:
> >   <TASK>
> >   dump_stack_lvl+0xa9/0xc0
> >   lockdep_rcu_suspicious+0x117/0x190
> >   cipso_v4_sock_setattr+0x1ab/0x1b0
> >   netlbl_sock_setattr+0x13e/0x1b0
> >   selinux_netlbl_socket_post_create+0x3f/0x80
> >   selinux_socket_post_create+0x1a0/0x460
> >   security_socket_post_create+0x42/0x60
> >   __sock_create+0x342/0x3a0
> >   __sys_socket_create.part.22+0x42/0x70
> >   __sys_socket+0x37/0xb0
> >   __x64_sys_socket+0x16/0x20
> >   do_syscall_64+0x96/0x180
> >   ? do_user_addr_fault+0x68d/0xa30
> >   ? exc_page_fault+0x171/0x280
> >   ? asm_exc_page_fault+0x22/0x30
> >   entry_SYSCALL_64_after_hwframe+0x71/0x79
> >  RIP: 0033:0x7fbc0ca3fc1b
> >  Code: 73 01 c3 48 8b 0d 05 f2 1b 00 f7 d8 64 89 01 48 83 c8 ff c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 29 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d5 f1 1b 00 f7 d8 64 89 01 48
> >  RSP: 002b:00007fff18635208 EFLAGS: 00000246 ORIG_RAX: 0000000000000029
> >  RAX: ffffffffffffffda RBX: 0000000000000001 RCX: 00007fbc0ca3fc1b
> >  RDX: 0000000000000006 RSI: 0000000000000001 RDI: 0000000000000002
> >  RBP: 000055d24f80f8a0 R08: 0000000000000003 R09: 0000000000000001
> >  R10: 0000000000020000 R11: 0000000000000246 R12: 000055d24f80f8a0
> >  R13: 0000000000000000 R14: 000055d24f80fb88 R15: 0000000000000000
> >   </TASK>
> >
> > The current implementation of cipso_v4_sock_setattr() replaces IP options
> > under the assumption that the caller holds the socket lock; however, such
> > assumption is not true, nor needed, in selinux_socket_post_create() hook.
> >
> > Using rcu_dereference_check() instead of rcu_dereference_protected() will
> > avoid the reported splat for the netlbl_sock_setattr() case, and preserve
> > the legitimate check when the caller is netlbl_conn_setattr().
> >
> > Fixes: f6d8bd051c39 ("inet: add RCU protection to inet->opt")
> > Reported-by: Xiumei Mu <xmu@redhat.com>
> > Suggested-by: Paolo Abeni <pabeni@redhat.com>
> > Signed-off-by: Davide Caratti <dcaratti@redhat.com>
> > ---
> >  net/ipv4/cipso_ipv4.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> > index 8b17d83e5fde..1d0c2a905078 100644
> > --- a/net/ipv4/cipso_ipv4.c
> > +++ b/net/ipv4/cipso_ipv4.c
> > @@ -1876,8 +1876,10 @@ int cipso_v4_sock_setattr(struct sock *sk,
> >
> >         sk_inet = inet_sk(sk);
> >
> > -       old = rcu_dereference_protected(sk_inet->inet_opt,
> > -                                       lockdep_sock_is_held(sk));
> > +       /* caller either holds rcu_read_lock() (on socket creation)
> > +        * or socket lock (in all other cases). */
> > +       old = rcu_dereference_check(sk_inet->inet_opt,
> > +                                   lockdep_sock_is_held(sk));
> >         if (inet_test_bit(IS_ICSK, sk)) {
> >                 sk_conn = inet_csk(sk);
> >                 if (old)
> > --
> > 2.44.0
> >
>
> OK, but rcu_read_lock() being held (incidentally by the caller) here
> is not protecting the write operation,
> so this looks wrong IMO.
>
> Whenever we can not ensure a mutex/spinlock is held, we usually use
> rcu_dereference_protected(XXX, 1),
> and a comment might simply explain the reason we assert it is protected.
>
> (We also could add a new boolean parameter, set to true or false
> depending on the caller)
>
> old = rcu_dereference_protected(sk_inet->inet_opt, from_socket_creation ||
>
>             lockdep_sock_is_held(sk));

Agree with everything Eric said.  I'm okay with the
rcu_deref_prot(XXX, 1) + comment approach, but if you wanted to put in
the work to chase the callers and setup the boolean that would be
great too.

-- 
paul-moore.com

Re: [PATCH net] netlabel: fix RCU annotation for IPv4 options on socket creation

[Paul Moore]

On Thu, Apr 11, 2024 at 1:41 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Please be sure to verify that this is appropriate for all users of netlabel.
> SELinux is not the only user of netlabel.

Adding my support to Casey's comment above.  If you go the boolean
route, please work with Casey to ensure that the Smack usage is
properly handled.

-- 
paul-moore.com