bpf: sockmap, fix skb refcnt race after locking changes

PATCH Applied John Fastabend 6 participants 7 messages Sep 1, 2023 – Sep 4, 2023

Subsystems (4)

Networking Network drivers BPF BPF sockmap

lore

Share & tools

lore

John Fastabend

author maintainer

2023-09-01 20:21 UTC #0 lore raw

PATCH 1/1 bpf: sockmap, fix skb refcnt race after locking changes

There is a race where skb's from the sk_psock_backlog can be referenced after userspace side has already skb_consumed() the sk_buff and its refcnt dropped to zer0 causing use after free.

The flow is the following,

while ((skb = skb_peek(&psock->ingress_skb))

    sk_psock_handle_Skb(psock, skb, ..., ingress)
    if (!ingress) ...
    sk_psock_skb_ingress
       sk_psock_skb_ingress_enqueue(skb)
          msg->skb = skb
          sk_psock_queue_msg(psock, msg)
    skb_dequeue(&psock->ingress_skb)

The sk_psock_queue_msg() puts the msg on the ingress_msg queue. This is what the application reads when recvmsg() is called. An application can read this anytime after the msg is placed on the queue. The recvmsg hook will also read msg->skb and then after user space reads the msg will call consume_skb(skb) on it effectively free'ing it.

But, the race is in above where backlog queue still has a reference to the skb and calls skb_dequeue(). If the skb_dequeue happens after the user reads and free's the skb we have a use after free.

The !ingress case does not suffer from this problem because it uses sendmsg_*(sk, msg) which does not pass the sk_buff further down the stack.

The following splat was observed with 'test_progs -t sockmap_listen':

[ 1022.710250][ T2556] general protection fault, ...
 ...
[ 1022.712830][ T2556] Workqueue: events sk_psock_backlog
[ 1022.713262][ T2556] RIP: 0010:skb_dequeue+0x4c/0x80
[ 1022.713653][ T2556] Code: ...
 ...
[ 1022.720699][ T2556] Call Trace:
[ 1022.720984][ T2556]  <TASK>
[ 1022.721254][ T2556]  ? die_addr+0x32/0x80^M
[ 1022.721589][ T2556]  ? exc_general_protection+0x25a/0x4b0
[ 1022.722026][ T2556]  ? asm_exc_general_protection+0x22/0x30
[ 1022.722489][ T2556]  ? skb_dequeue+0x4c/0x80
[ 1022.722854][ T2556]  sk_psock_backlog+0x27a/0x300
[ 1022.723243][ T2556]  process_one_work+0x2a7/0x5b0
[ 1022.723633][ T2556]  worker_thread+0x4f/0x3a0
[ 1022.723998][ T2556]  ? __pfx_worker_thread+0x10/0x10
[ 1022.724386][ T2556]  kthread+0xfd/0x130
[ 1022.724709][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725066][ T2556]  ret_from_fork+0x2d/0x50
[ 1022.725409][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725799][ T2556]  ret_from_fork_asm+0x1b/0x30
[ 1022.726201][ T2556]  </TASK>

To fix we add an skb_get() before passing the skb to be enqueued in the engress queue. This bumps the skb->users refcnt so that consume_skb and kfree_skb will not immediately free the sk_buff. With this we can be sure the skb is still around when we do the dequeue. Then we just need to decrement the refcnt or free the skb in the backlog case which we do by calling kfree_skb() on the ingress case as well as the sendmsg case.

Before locking change from fixes tag we had the sock locked so we couldn't race with user and there was no issue here.

Fixes 799aa7f98d53e (skmsg: Avoid lock_sock() in sk_psock_backlog())

Reported-by Jiri Olsa <[email protected]>

Signed-off-by John Fastabend <[email protected]>

---
 net/core/skmsg.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

net/core/skmsg.c +8 -4

index a0659fc29bcc..6c31eefbd777 100644
@@ -612,12 +612,18 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
 static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
 			       u32 off, u32 len, bool ingress)
 {
+	int err = 0;
+
 	if (!ingress) {
 		if (!sock_writeable(psock->sk))
 			return -EAGAIN;
 		return skb_send_sock(psock->sk, skb, off, len);
 	}
-	return sk_psock_skb_ingress(psock, skb, off, len);
+	skb_get(skb);
+	err = sk_psock_skb_ingress(psock, skb, off, len);
+	if (err < 0)
+		kfree_skb(skb);
+	return err;
 }
 
 static void sk_psock_skb_state(struct sk_psock *psock,

@@ -685,9 +691,7 @@ static void sk_psock_backlog(struct work_struct *work)
 		} while (len);
 
 		skb = skb_dequeue(&psock->ingress_skb);
-		if (!ingress) {
-			kfree_skb(skb);
-		}
+		kfree_skb(skb);
 	}
 end:
 	mutex_unlock(&psock->work_mutex);

Signature · 1 line

2.33.0

Jiri Olsa

2023-09-01 21:20 UTC #1 lore raw

Quoted 110 lines · John Fastabend

There is a race where skb's from the sk_psock_backlog can be referenced after userspace side has already skb_consumed() the sk_buff and its refcnt dropped to zer0 causing use after free.
The flow is the following,
while ((skb = skb_peek(&psock->ingress_skb))
    sk_psock_handle_Skb(psock, skb, ..., ingress)
    if (!ingress) ...
    sk_psock_skb_ingress
       sk_psock_skb_ingress_enqueue(skb)
          msg->skb = skb
          sk_psock_queue_msg(psock, msg)
    skb_dequeue(&psock->ingress_skb)
The sk_psock_queue_msg() puts the msg on the ingress_msg queue. This is what the application reads when recvmsg() is called. An application can read this anytime after the msg is placed on the queue. The recvmsg hook will also read msg->skb and then after user space reads the msg will call consume_skb(skb) on it effectively free'ing it.
But, the race is in above where backlog queue still has a reference to the skb and calls skb_dequeue(). If the skb_dequeue happens after the user reads and free's the skb we have a use after free.
The !ingress case does not suffer from this problem because it uses sendmsg_*(sk, msg) which does not pass the sk_buff further down the stack.
The following splat was observed with 'test_progs -t sockmap_listen':
[ 1022.710250][ T2556] general protection fault, ...
 ...
[ 1022.712830][ T2556] Workqueue: events sk_psock_backlog
[ 1022.713262][ T2556] RIP: 0010:skb_dequeue+0x4c/0x80
[ 1022.713653][ T2556] Code: ...
 ...
[ 1022.720699][ T2556] Call Trace:
[ 1022.720984][ T2556]  <TASK>
[ 1022.721254][ T2556]  ? die_addr+0x32/0x80^M
[ 1022.721589][ T2556]  ? exc_general_protection+0x25a/0x4b0
[ 1022.722026][ T2556]  ? asm_exc_general_protection+0x22/0x30
[ 1022.722489][ T2556]  ? skb_dequeue+0x4c/0x80
[ 1022.722854][ T2556]  sk_psock_backlog+0x27a/0x300
[ 1022.723243][ T2556]  process_one_work+0x2a7/0x5b0
[ 1022.723633][ T2556]  worker_thread+0x4f/0x3a0
[ 1022.723998][ T2556]  ? __pfx_worker_thread+0x10/0x10
[ 1022.724386][ T2556]  kthread+0xfd/0x130
[ 1022.724709][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725066][ T2556]  ret_from_fork+0x2d/0x50
[ 1022.725409][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725799][ T2556]  ret_from_fork_asm+0x1b/0x30
[ 1022.726201][ T2556]  </TASK>
To fix we add an skb_get() before passing the skb to be enqueued in the engress queue. This bumps the skb->users refcnt so that consume_skb and kfree_skb will not immediately free the sk_buff. With this we can be sure the skb is still around when we do the dequeue. Then we just need to decrement the refcnt or free the skb in the backlog case which we do by calling kfree_skb() on the ingress case as well as the sendmsg case.
Before locking change from fixes tag we had the sock locked so we couldn't race with user and there was no issue here.
Fixes 799aa7f98d53e (skmsg: Avoid lock_sock() in sk_psock_backlog())
Reported-by Jiri Olsa <[email protected]>
Signed-off-by John Fastabend <[email protected]>
---
 net/core/skmsg.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
net/core/skmsg.c +8 -4
index a0659fc29bcc..6c31eefbd777 100644
@@ -612,12 +612,18 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
 static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
 			       u32 off, u32 len, bool ingress)
 {
+	int err = 0;
+
 	if (!ingress) {
 		if (!sock_writeable(psock->sk))
 			return -EAGAIN;
 		return skb_send_sock(psock->sk, skb, off, len);
 	}
-	return sk_psock_skb_ingress(psock, skb, off, len);
+	skb_get(skb);
+	err = sk_psock_skb_ingress(psock, skb, off, len);
+	if (err < 0)
+		kfree_skb(skb);
+	return err;
 }
 
 static void sk_psock_skb_state(struct sk_psock *psock,
@@ -685,9 +691,7 @@ static void sk_psock_backlog(struct work_struct *work)
 		} while (len);
 
 		skb = skb_dequeue(&psock->ingress_skb);
-		if (!ingress) {
-			kfree_skb(skb);
-		}
+		kfree_skb(skb);
 	}
 end:
 	mutex_unlock(&psock->work_mutex);
Signature · 1 line
2.33.0

there's no crash wit with fix, but I noticed I occasionally get FAIL

#212/78 sockmap_listen/sockmap Unix test_unix_redir:OK ./test_progs:vsock_unix_redir_connectible:1501: ingress: write: Transport endpoint is not connected vsock_unix_redir_connectible:FAIL:1501 ./test_progs:vsock_unix_redir_connectible:1501: egress: write: Transport endpoint is not connected vsock_unix_redir_connectible:FAIL:1501
#212/79 sockmap_listen/sockmap VSOCK test_vsock_redir:FAIL
#212/80 sockmap_listen/sockhash IPv4 TCP test_insert_invalid:OK

no idea if it's related

jirka

Eduard Zingerman

maintainer

2023-09-01 21:39 UTC #2 lore raw

Quoted 100 lines · Jiri Olsa

Quoted 99 lines · John Fastabend
There is a race where skb's from the sk_psock_backlog can be referenced after userspace side has already skb_consumed() the sk_buff and its refcnt dropped to zer0 causing use after free.
The flow is the following,
while ((skb = skb_peek(&psock->ingress_skb))
    sk_psock_handle_Skb(psock, skb, ..., ingress)
    if (!ingress) ...
    sk_psock_skb_ingress
       sk_psock_skb_ingress_enqueue(skb)
          msg->skb = skb
          sk_psock_queue_msg(psock, msg)
    skb_dequeue(&psock->ingress_skb)
The sk_psock_queue_msg() puts the msg on the ingress_msg queue. This is what the application reads when recvmsg() is called. An application can read this anytime after the msg is placed on the queue. The recvmsg hook will also read msg->skb and then after user space reads the msg will call consume_skb(skb) on it effectively free'ing it.
But, the race is in above where backlog queue still has a reference to the skb and calls skb_dequeue(). If the skb_dequeue happens after the user reads and free's the skb we have a use after free.
The !ingress case does not suffer from this problem because it uses sendmsg_*(sk, msg) which does not pass the sk_buff further down the stack.
The following splat was observed with 'test_progs -t sockmap_listen':
[ 1022.710250][ T2556] general protection fault, ...
 ...
[ 1022.712830][ T2556] Workqueue: events sk_psock_backlog
[ 1022.713262][ T2556] RIP: 0010:skb_dequeue+0x4c/0x80
[ 1022.713653][ T2556] Code: ...
 ...
[ 1022.720699][ T2556] Call Trace:
[ 1022.720984][ T2556]  <TASK>
[ 1022.721254][ T2556]  ? die_addr+0x32/0x80^M
[ 1022.721589][ T2556]  ? exc_general_protection+0x25a/0x4b0
[ 1022.722026][ T2556]  ? asm_exc_general_protection+0x22/0x30
[ 1022.722489][ T2556]  ? skb_dequeue+0x4c/0x80
[ 1022.722854][ T2556]  sk_psock_backlog+0x27a/0x300
[ 1022.723243][ T2556]  process_one_work+0x2a7/0x5b0
[ 1022.723633][ T2556]  worker_thread+0x4f/0x3a0
[ 1022.723998][ T2556]  ? __pfx_worker_thread+0x10/0x10
[ 1022.724386][ T2556]  kthread+0xfd/0x130
[ 1022.724709][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725066][ T2556]  ret_from_fork+0x2d/0x50
[ 1022.725409][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725799][ T2556]  ret_from_fork_asm+0x1b/0x30
[ 1022.726201][ T2556]  </TASK>
To fix we add an skb_get() before passing the skb to be enqueued in the engress queue. This bumps the skb->users refcnt so that consume_skb and kfree_skb will not immediately free the sk_buff. With this we can be sure the skb is still around when we do the dequeue. Then we just need to decrement the refcnt or free the skb in the backlog case which we do by calling kfree_skb() on the ingress case as well as the sendmsg case.
Before locking change from fixes tag we had the sock locked so we couldn't race with user and there was no issue here.
Fixes 799aa7f98d53e (skmsg: Avoid lock_sock() in sk_psock_backlog())
Reported-by Jiri Olsa <[email protected]>
Signed-off-by John Fastabend <[email protected]>
---
 net/core/skmsg.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
net/core/skmsg.c +7 -1
index a0659fc29bcc..6c31eefbd777 100644
@@ -612,12 +612,18 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
 static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
 			       u32 off, u32 len, bool ingress)
 {
+	int err = 0;
+
 	if (!ingress) {
 		if (!sock_writeable(psock->sk))
 			return -EAGAIN;
 		return skb_send_sock(psock->sk, skb, off, len);
 	}
-	return sk_psock_skb_ingress(psock, skb, off, len);
+	skb_get(skb);
+	err = sk_psock_skb_ingress(psock, skb, off, len);
+	if (err < 0)
+		kfree_skb(skb);
+	return err;
 }
 
 static void sk_psock_skb_state(struct sk_psock *psock,
@@ -685,9 +691,7 @@ static void sk_psock_backlog(struct work_struct *work)
 		} while (len);
 

Quoted 14 lines

Quoted 11 lines
net/core/skmsg.c +1 -3
 		skb = skb_dequeue(&psock->ingress_skb);
-		if (!ingress) {
-			kfree_skb(skb);
-		}
+		kfree_skb(skb);
 	}
 end:
 	mutex_unlock(&psock->work_mutex);
Signature · 1 line
2.33.0
there's no crash wit with fix, but I noticed I occasionally get FAIL

Please note this patch:
https://lore.kernel.org/bpf/[email protected]/
Which should fix the test in question.

Quoted 11 lines

#212/78 sockmap_listen/sockmap Unix test_unix_redir:OK ./test_progs:vsock_unix_redir_connectible:1501: ingress: write: Transport endpoint is not connected vsock_unix_redir_connectible:FAIL:1501 ./test_progs:vsock_unix_redir_connectible:1501: egress: write: Transport endpoint is not connected vsock_unix_redir_connectible:FAIL:1501
#212/79 sockmap_listen/sockmap VSOCK test_vsock_redir:FAIL
#212/80 sockmap_listen/sockhash IPv4 TCP test_insert_invalid:OK
no idea if it's related
jirka

Jiri Olsa

Tested-by

2023-09-02 17:28 UTC #3 lore raw

On Sat, Sep 02, 2023 at 12:24:01AM +0300, Eduard Zingerman wrote:

SNIP

Quoted 19 lines

Quoted 4 lines
Quoted 4 lines
static void sk_psock_skb_state(struct sk_psock *psock,
net/core/skmsg.c +0 -0
@@ -685,9 +691,7 @@ static void sk_psock_backlog(struct work_struct *work)
 		} while (len);
 
Quoted 14 lines
Quoted 11 lines
net/core/skmsg.c +1 -3
 		skb = skb_dequeue(&psock->ingress_skb);
-		if (!ingress) {
-			kfree_skb(skb);
-		}
+		kfree_skb(skb);
 	}
 end:
 	mutex_unlock(&psock->work_mutex);
Signature · 1 line
2.33.0
there's no crash wit with fix, but I noticed I occasionally get FAIL

Quoted 3 lines

Please note this patch:
https://lore.kernel.org/bpf/[email protected]/
Which should fix the test in question.

ah right it does, thanks

Tested-by Jiri Olsa <[email protected]>

jirka

Quoted 13 lines

Quoted 12 lines
#212/78 sockmap_listen/sockmap Unix test_unix_redir:OK ./test_progs:vsock_unix_redir_connectible:1501: ingress: write: Transport endpoint is not connected vsock_unix_redir_connectible:FAIL:1501 ./test_progs:vsock_unix_redir_connectible:1501: egress: write: Transport endpoint is not connected vsock_unix_redir_connectible:FAIL:1501
#212/79 sockmap_listen/sockmap VSOCK test_vsock_redir:FAIL
#212/80 sockmap_listen/sockhash IPv4 TCP test_insert_invalid:OK
no idea if it's related
jirka

Xu Kuohai

Tested-by on v1 1/1

2023-09-02 08:13 UTC #4 lore raw

In reply to John Fastabend

Quoted 97 lines · John Fastabend

There is a race where skb's from the sk_psock_backlog can be referenced after userspace side has already skb_consumed() the sk_buff and its refcnt dropped to zer0 causing use after free.
The flow is the following,
   while ((skb = skb_peek(&psock->ingress_skb))
     sk_psock_handle_Skb(psock, skb, ..., ingress)
     if (!ingress) ...
     sk_psock_skb_ingress
        sk_psock_skb_ingress_enqueue(skb)
           msg->skb = skb
           sk_psock_queue_msg(psock, msg)
     skb_dequeue(&psock->ingress_skb)
The sk_psock_queue_msg() puts the msg on the ingress_msg queue. This is what the application reads when recvmsg() is called. An application can read this anytime after the msg is placed on the queue. The recvmsg hook will also read msg->skb and then after user space reads the msg will call consume_skb(skb) on it effectively free'ing it.
But, the race is in above where backlog queue still has a reference to the skb and calls skb_dequeue(). If the skb_dequeue happens after the user reads and free's the skb we have a use after free.
The !ingress case does not suffer from this problem because it uses sendmsg_*(sk, msg) which does not pass the sk_buff further down the stack.
The following splat was observed with 'test_progs -t sockmap_listen':
[ 1022.710250][ T2556] general protection fault, ...
  ...
[ 1022.712830][ T2556] Workqueue: events sk_psock_backlog
[ 1022.713262][ T2556] RIP: 0010:skb_dequeue+0x4c/0x80
[ 1022.713653][ T2556] Code: ...
  ...
[ 1022.720699][ T2556] Call Trace:
[ 1022.720984][ T2556]  <TASK>
[ 1022.721254][ T2556]  ? die_addr+0x32/0x80^M
[ 1022.721589][ T2556]  ? exc_general_protection+0x25a/0x4b0
[ 1022.722026][ T2556]  ? asm_exc_general_protection+0x22/0x30
[ 1022.722489][ T2556]  ? skb_dequeue+0x4c/0x80
[ 1022.722854][ T2556]  sk_psock_backlog+0x27a/0x300
[ 1022.723243][ T2556]  process_one_work+0x2a7/0x5b0
[ 1022.723633][ T2556]  worker_thread+0x4f/0x3a0
[ 1022.723998][ T2556]  ? __pfx_worker_thread+0x10/0x10
[ 1022.724386][ T2556]  kthread+0xfd/0x130
[ 1022.724709][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725066][ T2556]  ret_from_fork+0x2d/0x50
[ 1022.725409][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725799][ T2556]  ret_from_fork_asm+0x1b/0x30
[ 1022.726201][ T2556]  </TASK>
To fix we add an skb_get() before passing the skb to be enqueued in the engress queue. This bumps the skb->users refcnt so that consume_skb and kfree_skb will not immediately free the sk_buff. With this we can be sure the skb is still around when we do the dequeue. Then we just need to decrement the refcnt or free the skb in the backlog case which we do by calling kfree_skb() on the ingress case as well as the sendmsg case.
Before locking change from fixes tag we had the sock locked so we couldn't race with user and there was no issue here.
Fixes 799aa7f98d53e (skmsg: Avoid lock_sock() in sk_psock_backlog())
Reported-by Jiri Olsa <[email protected]>
Signed-off-by John Fastabend <[email protected]>
---
  net/core/skmsg.c | 12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)
net/core/skmsg.c +7 -1
index a0659fc29bcc..6c31eefbd777 100644
@@ -612,12 +612,18 @@ static int sk_psock_skb_ingress_self(struct sk_psock *psock, struct sk_buff *skb
  static int sk_psock_handle_skb(struct sk_psock *psock, struct sk_buff *skb,
  			       u32 off, u32 len, bool ingress)
  {
+	int err = 0;
+
  	if (!ingress) {
  		if (!sock_writeable(psock->sk))
  			return -EAGAIN;
  		return skb_send_sock(psock->sk, skb, off, len);
  	}
-	return sk_psock_skb_ingress(psock, skb, off, len);
+	skb_get(skb);
+	err = sk_psock_skb_ingress(psock, skb, off, len);
+	if (err < 0)
+		kfree_skb(skb);
+	return err;
  }
 
  static void sk_psock_skb_state(struct sk_psock *psock,

net/core/skmsg.c static void sk_psock_backlog(struct work_struct *work) · line 697

Quoted 10 lines

net/core/skmsg.c +1 -3

@@ -685,9 +691,7 @@ static void sk_psock_backlog(struct work_struct *work)
  		} while (len);
 
  		skb = skb_dequeue(&psock->ingress_skb);
-		if (!ingress) {
-			kfree_skb(skb);
-		}
+		kfree_skb(skb);
  	}
  end:
  	mutex_unlock(&psock->work_mutex);

Tested-by Xu Kuohai <[email protected]>

Simon Horman

reviewer

2023-09-02 09:00 UTC #5 lore raw

In reply to John Fastabend

Quoted 62 lines · John Fastabend

There is a race where skb's from the sk_psock_backlog can be referenced after userspace side has already skb_consumed() the sk_buff and its refcnt dropped to zer0 causing use after free.
The flow is the following,
while ((skb = skb_peek(&psock->ingress_skb))
    sk_psock_handle_Skb(psock, skb, ..., ingress)
    if (!ingress) ...
    sk_psock_skb_ingress
       sk_psock_skb_ingress_enqueue(skb)
          msg->skb = skb
          sk_psock_queue_msg(psock, msg)
    skb_dequeue(&psock->ingress_skb)
The sk_psock_queue_msg() puts the msg on the ingress_msg queue. This is what the application reads when recvmsg() is called. An application can read this anytime after the msg is placed on the queue. The recvmsg hook will also read msg->skb and then after user space reads the msg will call consume_skb(skb) on it effectively free'ing it.
But, the race is in above where backlog queue still has a reference to the skb and calls skb_dequeue(). If the skb_dequeue happens after the user reads and free's the skb we have a use after free.
The !ingress case does not suffer from this problem because it uses sendmsg_*(sk, msg) which does not pass the sk_buff further down the stack.
The following splat was observed with 'test_progs -t sockmap_listen':
[ 1022.710250][ T2556] general protection fault, ...
 ...
[ 1022.712830][ T2556] Workqueue: events sk_psock_backlog
[ 1022.713262][ T2556] RIP: 0010:skb_dequeue+0x4c/0x80
[ 1022.713653][ T2556] Code: ...
 ...
[ 1022.720699][ T2556] Call Trace:
[ 1022.720984][ T2556]  <TASK>
[ 1022.721254][ T2556]  ? die_addr+0x32/0x80^M
[ 1022.721589][ T2556]  ? exc_general_protection+0x25a/0x4b0
[ 1022.722026][ T2556]  ? asm_exc_general_protection+0x22/0x30
[ 1022.722489][ T2556]  ? skb_dequeue+0x4c/0x80
[ 1022.722854][ T2556]  sk_psock_backlog+0x27a/0x300
[ 1022.723243][ T2556]  process_one_work+0x2a7/0x5b0
[ 1022.723633][ T2556]  worker_thread+0x4f/0x3a0
[ 1022.723998][ T2556]  ? __pfx_worker_thread+0x10/0x10
[ 1022.724386][ T2556]  kthread+0xfd/0x130
[ 1022.724709][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725066][ T2556]  ret_from_fork+0x2d/0x50
[ 1022.725409][ T2556]  ? __pfx_kthread+0x10/0x10
[ 1022.725799][ T2556]  ret_from_fork_asm+0x1b/0x30
[ 1022.726201][ T2556]  </TASK>
To fix we add an skb_get() before passing the skb to be enqueued in the engress queue. This bumps the skb->users refcnt so that consume_skb and kfree_skb will not immediately free the sk_buff. With this we can be sure the skb is still around when we do the dequeue. Then we just need to decrement the refcnt or free the skb in the backlog case which we do by calling kfree_skb() on the ingress case as well as the sendmsg case.

Quoted 4 lines

Before locking change from fixes tag we had the sock locked so we couldn't race with user and there was no issue here.
Fixes 799aa7f98d53e (skmsg: Avoid lock_sock() in sk_psock_backlog())

Hi John,

A minor nit from my side.
I think the usual format for a fixes tag is follows.

Fixes 799aa7f98d53e ("skmsg: Avoid lock_sock() in sk_psock_backlog()")

Quoted 2 lines

Reported-by Jiri Olsa <[email protected]>
Signed-off-by John Fastabend <[email protected]>

...

[email protected]

CI Applied

2023-09-04 08:13 UTC #6 lore raw

In reply to John Fastabend

This patch was applied to bpf/bpf.git (master) by Daniel Borkmann <[email protected]>:

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <[email protected]>:

Quoted 15 lines · On Fri, 1 Sep 2023 13:21:37 -0700 you wrote:

There is a race where skb's from the sk_psock_backlog can be referenced after userspace side has already skb_consumed() the sk_buff and its refcnt dropped to zer0 causing use after free.
The flow is the following,
while ((skb = skb_peek(&psock->ingress_skb))
    sk_psock_handle_Skb(psock, skb, ..., ingress)
    if (!ingress) ...
    sk_psock_skb_ingress
       sk_psock_skb_ingress_enqueue(skb)
          msg->skb = skb
          sk_psock_queue_msg(psock, msg)
    skb_dequeue(&psock->ingress_skb)

Quoted 1 line

[...]

Here is the summary with links:
- [bpf] bpf: sockmap, fix skb refcnt race after locking changes

    https://git.kernel.org/bpf/bpf/c/a454d84ee20b

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html