On Wed, Aug 30, 2023 at 08:58:09PM +0200, Magnus Karlsson wrote:

On Wed, 30 Aug 2023 at 19:03, Maciej Fijalkowski
[off-list ref] wrote:

quoted

On Wed, Aug 30, 2023 at 05:17:03PM +0200, Magnus Karlsson wrote:

quoted

From: Magnus Karlsson <magnus.karlsson@intel.com>

Fix a use-after-free error that is possible if the xsk_diag interface
is used at the same time as the socket is being closed. In the early

I thought our understanding is: socket is alive, we use diag interface
against it but netdev that we bound socket to is being torn down.

If the socket was not going down at the same time, we would still have
a reference to the netdevice and it would not disappear. So the socket
needs to be going down for this to happen.

No, I am able to trigger this now on my local system with KASAN turned on
via:

window 0:
sudo ./xdpsock -i enp24s0f0np0 -r -z -q 17

window 1:
watch -n 0.1 "ss --xdp -e"

window 2:
sudo rmmod ice

we hold the device via dev_get_by_index() in xsk_bind() but dev_put() is
called from xsk_unbind_dev() which can happen either from xsk_release() or
xsk_notifier(), our case refers to the latter.

I don't see currently how ss gets the ifname but after rmmoding ice I am
getting something bogus over there:

Recv-Q Send-Q Local Address:Port Peer Address:PortProcess
0      0               if18:q17              *     ino:18691 sk:2001
        rx(entries:2048)
        umem(id:0,size:16777216,num_pages:4096,chunk_size:4096,headroom:0,ifindex:0,qid:17,zc:1,refs:1)
        fr(entries:4096)
        cr(entries:2048)
        stats(rx dropped:0,rx invalid:0,rx queue full:0,rx fill ring empty:0,tx invalid:0,tx ring empty:0)

'if18' instead 'enp24s0f0np0'. With your patch we bail out early so we
wouldn't have that problem AFAICT.

quoted

quoted

days of AF_XDP, the way we tested that a socket was not bound or being
closed was to simply check if the netdevice pointer in the xsk socket
structure was NULL. Later, a better system was introduced by having an
explicit state variable in the xsk socket struct. For example, the
state of a socket that is going down is XSK_UNBOUND.

The commit in the Fixes tag below deleted the old way of signalling
that a socket is going down, setting dev to NULL. This in the belief
that all code using the old way had been exterminated. That was
unfortunately not true as the xsk diagnostics code was still using the
old way and thus does not work as intended when a socket is going
down. Fix this by introducing a test against the state variable. If

Again, I believe it was not the socket going down but rather the netdev?

quoted

the socket is going down, simply abort the diagnostic's netlink
operation.

Fixes: 18b1ab7aa76b ("xsk: Fix race at socket teardown")
Reported-by: syzbot+822d1359297e2694f873@syzkaller.appspotmail.com

Nit: I see syzbot wanted you to include:
Reported-and-tested-by: syzbot+822d13...@syzkaller.appspotmail.com

quoted

Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
---
 net/xdp/xsk_diag.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/xdp/xsk_diag.c b/net/xdp/xsk_diag.c
index c014217f5fa7..da3100bfa1c5 100644
--- a/net/xdp/xsk_diag.c
+++ b/net/xdp/xsk_diag.c

@@ -111,6 +111,9 @@ static int xsk_diag_fill(struct sock *sk, struct sk_buff *nlskb,
      sock_diag_save_cookie(sk, msg->xdiag_cookie);

      mutex_lock(&xs->mutex);
+     if (xs->state == XSK_UNBOUND)
+             goto out_nlmsg_trim;

With the above I feel like we can get rid of xs->dev test in
xsk_diag_put_info(), no?

It has to stay since the socket does not get a reference to the device
until it is bound. It is fine to use the xsk_diag interface on an
unbound socket to query its state.

Yes good point here.

quoted

quoted

+
      if ((req->xdiag_show & XDP_SHOW_INFO) && xsk_diag_put_info(xs, nlskb))
              goto out_nlmsg_trim;


base-commit: 35d2b7ffffc1d9b3dc6c761010aa3338da49165b
--
2.42.0