linux-kernel - Re: [PATCH bpf-next v2 1/3] bpf, sockmap: avoid using sk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <78ee737400721758fa67b4f285e8ba61dc6b893b@linux.dev>
Date: Mon, 10 Mar 2025 11:36:44 +0000
From: "Jiayuan Chen" <jiayuan.chen@...ux.dev>
To: "Michal Luczaj" <mhal@...x.co>, xiyou.wangcong@...il.com,
 john.fastabend@...il.com, jakub@...udflare.com, martin.lau@...ux.dev
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, horms@...nel.org, andrii@...nel.org,
 eddyz87@...il.com, mykolal@...com, ast@...nel.org, daniel@...earbox.net,
 song@...nel.org, yonghong.song@...ux.dev, kpsingh@...nel.org,
 sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org, shuah@...nel.org,
 sgarzare@...hat.com, netdev@...r.kernel.org, bpf@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
 mrpre@....com, cong.wang@...edance.com,
 syzbot+dd90a702f518e0eac072@...kaller.appspotmail.com
Subject: Re: [PATCH bpf-next v2 1/3] bpf, sockmap: avoid using sk_socket
 after free

March 7, 2025 at 5:45 PM, "Michal Luczaj" <mhal@...x.co> wrote:

> 
> On 2/28/25 06:51, Jiayuan Chen wrote:
> 
> > 
> > ...
> > 
> >  static void sk_psock_verdict_data_ready(struct sock *sk)
> > 
> >  {
> > 
> >  - struct socket *sock = sk->sk_socket;
> > 
> >  + struct socket *sock;
> > 
> >  const struct proto_ops *ops;
> > 
> >  int copied;
> > 
> >  
> > 
> >  trace_sk_data_ready(sk);
> > 
> >  
> > 
> >  + /* We need RCU to prevent the sk_socket from being released.
> > 
> >  + * Especially for Unix sockets, we are currently in the process
> > 
> >  + * context and do not have RCU protection.
> > 
> >  + */
> > 
> >  + rcu_read_lock();
> > 
> >  + sock = sk->sk_socket;
> > 
> >  if (unlikely(!sock))
> > 
> >  - return;
> > 
> >  + goto unlock;
> > 
> >  +
> > 
> >  ops = READ_ONCE(sock->ops);
> > 
> >  if (!ops || !ops->read_skb)
> > 
> >  - return;
> > 
> >  + goto unlock;
> > 
> >  +
> > 
> >  copied = ops->read_skb(sk, sk_psock_verdict_recv);
> > 
> >  if (copied >= 0) {
> > 
> >  struct sk_psock *psock;
> > 
> >  
> > 
> >  - rcu_read_lock();
> > 
> >  psock = sk_psock(sk);
> > 
> >  if (psock)
> > 
> >  sk_psock_data_ready(sk, psock);
> > 
> >  - rcu_read_unlock();
> > 
> >  }
> > 
> >  +unlock:
> > 
> >  + rcu_read_unlock();
> > 
> >  }
> > 
> 
> Hi,
> 
> Doesn't sk_psock_handle_skb() (!ingress path) have the same `struct socket`
> 
> release race issue? Any plans on fixing that one, too?

Yes, the send path logic also has similar issues, and after some hacking,
I was able to reproduce it. Thanks for providing this information.
I can fix these together in the next revision of this patchset, anyway,
this patchset still needs further confirmation from the maintainer.

> 
> BTW, lockdep (CONFIG_LOCKDEP=y) complains about calling AF_UNIX's
> 
> read_skb() under RCU read lock.
> 
> Thanks,
> 
> Michal
>
My environment also has LOCKDEP enabled, but I didn't see similar
warnings.
Moreover, RCU assertions are typically written as:

WARN_ON_ONCE(!rcu_read_lock_held())

And when LOCKDEP is not enabled, rcu_read_lock_held() defaults to
returning 1. So, it's unlikely to trigger a warning due to an RCU lock
being held.

Could you provide more of the call stack?

Thanks.