[bpf] xsk: fix crash on double free in buffer pool

Commit Message

Magnus Karlsson Nov. 11, 2021, 7:57 a.m. UTC

From: Magnus Karlsson <[e-mail]>

Fix a crash in the buffer pool allocator when a buffer is double
freed. It is possible to trigger this behavior not only from a faulty
driver, but also from user space like this: Create a zero-copy AF_XDP
socket. Load an XDP program that will issue XDP_DROP for all
packets. Put the same umem buffer into the fill ring multiple times,
then bind the socket and send some traffic. This will crash the kernel
as the XDP_DROP action triggers one call to xsk_buff_free()/xp_free()
for every packet dropped. Each call will add the corresponding buffer
entry to the free_list and increase the free_list_cnt. Some entries
will have been added multiple times due to the same buffer being
freed. The buffer allocation code will then traverse this broken list
and since the same buffer is in the list multiple times, it will try
to delete the same buffer twice from the list leading to a crash.

The fix for this is just to test that the buffer has not been added
before in xp_free(). If it has been, just return from the function and
do not put it in the free_list a second time.

Note that this bug was not present in the code before the commit
referenced in the Fixes tag. That code used one list entry per
allocated buffer, so multiple frees did not have any side effects. But
the commit below optimized the usage of the pool and only uses a
single entry per buffer in the umem, meaning that multiple
allocations/frees of the same buffer will also only use one entry,
thus leading to the problem.

Fixes: 47e4075df300 ("xsk: Batched buffer allocation for the pool")
Signed-off-by: Magnus Karlsson <[e-mail]>
---
 net/xdp/xsk_buff_pool.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


base-commit: fceb07950a7aac43d52d8c6ef580399a8b9b68fe

Comments

Björn Töpel Nov. 11, 2021, 4:53 p.m. UTC | #1

On Thu, 11 Nov 2021 at 08:57, Magnus Karlsson <[e-mail]> wrote:
>
> From: Magnus Karlsson <[e-mail]>
>
> Fix a crash in the buffer pool allocator when a buffer is double
> freed. It is possible to trigger this behavior not only from a faulty
> driver, but also from user space like this: Create a zero-copy AF_XDP
> socket. Load an XDP program that will issue XDP_DROP for all
> packets. Put the same umem buffer into the fill ring multiple times,
> then bind the socket and send some traffic. This will crash the kernel
> as the XDP_DROP action triggers one call to xsk_buff_free()/xp_free()
> for every packet dropped. Each call will add the corresponding buffer
> entry to the free_list and increase the free_list_cnt. Some entries
> will have been added multiple times due to the same buffer being
> freed. The buffer allocation code will then traverse this broken list
> and since the same buffer is in the list multiple times, it will try
> to delete the same buffer twice from the list leading to a crash.
>
> The fix for this is just to test that the buffer has not been added
> before in xp_free(). If it has been, just return from the function and
> do not put it in the free_list a second time.
>
> Note that this bug was not present in the code before the commit
> referenced in the Fixes tag. That code used one list entry per
> allocated buffer, so multiple frees did not have any side effects. But
> the commit below optimized the usage of the pool and only uses a
> single entry per buffer in the umem, meaning that multiple
> allocations/frees of the same buffer will also only use one entry,
> thus leading to the problem.
>
> Fixes: 47e4075df300 ("xsk: Batched buffer allocation for the pool")
> Signed-off-by: Magnus Karlsson <[e-mail]>

Acked-by: Björn Töpel <[e-mail]>

[e-mail] Nov. 12, 2021, 3 p.m. UTC | #2

Hello:

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

On Thu, 11 Nov 2021 08:57:07 +0100 you wrote:
> From: Magnus Karlsson <[e-mail]>
> 
> Fix a crash in the buffer pool allocator when a buffer is double
> freed. It is possible to trigger this behavior not only from a faulty
> driver, but also from user space like this: Create a zero-copy AF_XDP
> socket. Load an XDP program that will issue XDP_DROP for all
> packets. Put the same umem buffer into the fill ring multiple times,
> then bind the socket and send some traffic. This will crash the kernel
> as the XDP_DROP action triggers one call to xsk_buff_free()/xp_free()
> for every packet dropped. Each call will add the corresponding buffer
> entry to the free_list and increase the free_list_cnt. Some entries
> will have been added multiple times due to the same buffer being
> freed. The buffer allocation code will then traverse this broken list
> and since the same buffer is in the list multiple times, it will try
> to delete the same buffer twice from the list leading to a crash.
> 
> [...]

Here is the summary with links:
  - [bpf] xsk: fix crash on double free in buffer pool
    https://git.kernel.org/bpf/bpf/c/199d983bc015

You are awesome, thank you!

Patch

diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 90c4e1e819d3..bc4ad48ea4f0 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -500,7 +500,7 @@  struct xdp_buff *xp_alloc(struct xsk_buff_pool *pool)
 		pool->free_list_cnt--;
 		xskb = list_first_entry(&pool->free_list, struct xdp_buff_xsk,
 					free_list_node);
-		list_del(&xskb->free_list_node);
+		list_del_init(&xskb->free_list_node);
 	}
 
 	xskb->xdp.data = xskb->xdp.data_hard_start + XDP_PACKET_HEADROOM;
@@ -568,7 +568,7 @@  static u32 xp_alloc_reused(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u3
 	i = nb_entries;
 	while (i--) {
 		xskb = list_first_entry(&pool->free_list, struct xdp_buff_xsk, free_list_node);
-		list_del(&xskb->free_list_node);
+		list_del_init(&xskb->free_list_node);
 
 		*xdp = &xskb->xdp;
 		xdp++;
@@ -615,6 +615,9 @@  EXPORT_SYMBOL(xp_can_alloc);
 
 void xp_free(struct xdp_buff_xsk *xskb)
 {
+	if (!list_empty(&xskb->free_list_node))
+		return;
+
 	xskb->pool->free_list_cnt++;
 	list_add(&xskb->free_list_node, &xskb->pool->free_list);
 }