[bpf] xsk: fix missing validation for skb and unaligned mode

Commit Message

Magnus Karlsson June 17, 2021, 9:22 a.m. UTC

From: Magnus Karlsson <[e-mail]>

Fix a missing validation of a Tx descriptor when executing in skb mode
and the umem is in unaligned mode. A descriptor could point to a
buffer straddling the end of the umem, thus effectively tricking the
kernel to read outside the allowed umem region. This could lead to a
kernel crash if that part of memory is not mapped.

In zero-copy mode, the descriptor validation code rejects such
descriptors by checking a bit in the DMA address that tells us if the
next page is physically contiguous or not. For the last page in the
umem, this bit is not set, therefore any descriptor pointing to a
packet straddling this last page boundary will be rejected. However,
the skb path does not use this bit since it copies out data and can do
so to two different pages. (It also does not have the array of DMA
address, so it cannot even store this bit.) The code just returned
that the packet is always physically contiguous. But this is
unfortunately also returned for the last page in the umem, which means
that packets that cross the end of the umem are being allowed, which
they should not be.

Fix this by introducing a check for this in the SKB path only, not
penalizing the zero-copy path.

Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
Signed-off-by: Magnus Karlsson <[e-mail]>
---
 include/net/xsk_buff_pool.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)


base-commit: da5ac772cfe2a03058b0accfac03fad60c46c24d

Comments

Björn Töpel June 18, 2021, 1:43 p.m. UTC | #1

On Thu, 17 Jun 2021 at 11:23, Magnus Karlsson <[e-mail]> wrote:
>
> From: Magnus Karlsson <[e-mail]>
>
> Fix a missing validation of a Tx descriptor when executing in skb mode
> and the umem is in unaligned mode. A descriptor could point to a
> buffer straddling the end of the umem, thus effectively tricking the
> kernel to read outside the allowed umem region. This could lead to a
> kernel crash if that part of memory is not mapped.
>
> In zero-copy mode, the descriptor validation code rejects such
> descriptors by checking a bit in the DMA address that tells us if the
> next page is physically contiguous or not. For the last page in the
> umem, this bit is not set, therefore any descriptor pointing to a
> packet straddling this last page boundary will be rejected. However,
> the skb path does not use this bit since it copies out data and can do
> so to two different pages. (It also does not have the array of DMA
> address, so it cannot even store this bit.) The code just returned
> that the packet is always physically contiguous. But this is
> unfortunately also returned for the last page in the umem, which means
> that packets that cross the end of the umem are being allowed, which
> they should not be.
>
> Fix this by introducing a check for this in the SKB path only, not
> penalizing the zero-copy path.
>
> Fixes: 2b43470add8c ("xsk: Introduce AF_XDP buffer allocation API")
> Signed-off-by: Magnus Karlsson <[e-mail]>

Nice catch!

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

> ---
>  include/net/xsk_buff_pool.h | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
> index eaa8386dbc63..7a9a23e7a604 100644
> --- a/include/net/xsk_buff_pool.h
> +++ b/include/net/xsk_buff_pool.h
> @@ -147,11 +147,16 @@ static inline bool xp_desc_crosses_non_contig_pg(struct xsk_buff_pool *pool,
>  {
>         bool cross_pg = (addr & (PAGE_SIZE - 1)) + len > PAGE_SIZE;
>
> -       if (pool->dma_pages_cnt && cross_pg) {
> +       if (likely(!cross_pg))
> +               return false;
> +
> +       if (pool->dma_pages_cnt) {
>                 return !(pool->dma_pages[addr >> PAGE_SHIFT] &
>                          XSK_NEXT_PG_CONTIG_MASK);
>         }
> -       return false;
> +
> +       /* skb path */
> +       return addr + len > pool->addrs_cnt;
>  }
>
>  static inline u64 xp_aligned_extract_addr(struct xsk_buff_pool *pool, u64 addr)
>
> base-commit: da5ac772cfe2a03058b0accfac03fad60c46c24d
> --
> 2.29.0
>

[e-mail] June 18, 2021, 3 p.m. UTC | #2

Hello:

This patch was applied to bpf/bpf.git (refs/heads/master):

On Thu, 17 Jun 2021 11:22:55 +0200 you wrote:
> From: Magnus Karlsson <[e-mail]>
> 
> Fix a missing validation of a Tx descriptor when executing in skb mode
> and the umem is in unaligned mode. A descriptor could point to a
> buffer straddling the end of the umem, thus effectively tricking the
> kernel to read outside the allowed umem region. This could lead to a
> kernel crash if that part of memory is not mapped.
> 
> [...]

Here is the summary with links:
  - [bpf] xsk: fix missing validation for skb and unaligned mode
    https://git.kernel.org/bpf/bpf/c/2f99619820c2

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Patch

diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
index eaa8386dbc63..7a9a23e7a604 100644
--- a/include/net/xsk_buff_pool.h
+++ b/include/net/xsk_buff_pool.h
@@ -147,11 +147,16 @@  static inline bool xp_desc_crosses_non_contig_pg(struct xsk_buff_pool *pool,
 {
 	bool cross_pg = (addr & (PAGE_SIZE - 1)) + len > PAGE_SIZE;
 
-	if (pool->dma_pages_cnt && cross_pg) {
+	if (likely(!cross_pg))
+		return false;
+
+	if (pool->dma_pages_cnt) {
 		return !(pool->dma_pages[addr >> PAGE_SHIFT] &
 			 XSK_NEXT_PG_CONTIG_MASK);
 	}
-	return false;
+
+	/* skb path */
+	return addr + len > pool->addrs_cnt;
 }
 
 static inline u64 xp_aligned_extract_addr(struct xsk_buff_pool *pool, u64 addr)