Re: [PATCH net v1] net: Fix tuntap uninitialized value

[Jiayuan Chen]

March 28, 2025 at 05:08, "Willem de Bruijn" <willemdebruijn.kernel@xxxxxxxxx> wrote:

> 
> Jiayuan Chen wrote:
> 
> > 
> > Then tun/tap allocates an skb, it additionally allocates a prepad size
> >  (usually equal to NET_SKB_PAD) but leaves it uninitialized.
> >  bpf_xdp_adjust_head() may move skb->data forward, which may lead to an
> >  issue.
> >  Since the linear address is likely to be allocated from kmem_cache, it's
> >  unlikely to trigger a KMSAN warning. We need some tricks, such as forcing
> >  kmem_cache_shrink in __do_kmalloc_node, to reproduce the issue and trigger
> >  a KMSAN warning.
> > 
> >  Reported-by: syzbot+0e6ddb1ef80986bdfe64@xxxxxxxxxxxxxxxxxxxxxxxxx
> >  Closes: https://lore.kernel.org/all/00000000000067f65105edbd295d@xxxxxxxxxx/T/
> >  Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxx>
> > 
> >  ---
> > 
> >  drivers/net/tun.c | 2 ++
> > 
> >  1 file changed, 2 insertions(+)
> > 
> >  
> > 
> >  diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > 
> >  index f75f912a0225..111f83668b5e 100644
> > 
> >  --- a/drivers/net/tun.c
> > 
> >  +++ b/drivers/net/tun.c
> > 
> >  @@ -1463,6 +1463,7 @@ static struct sk_buff *tun_alloc_skb(struct tun_file *tfile,
> > 
> >  if (!skb)
> > 
> >  return ERR_PTR(err);
> > 
> >  
> > 
> >  + memset(skb->data, 0, prepad);
> > 
> >  skb_reserve(skb, prepad);
> > 
> >  skb_put(skb, linear);
> > 
> >  skb->data_len = len - linear;
> > 
> 
> Is this specific to the tun device?
> 
> This happens in generic (skb) xdp.
> 
> The stackdump shows a napi poll call stack
> 
>  bpf_prog_run_generic_xdp+0x13ff/0x1a30 net/core/dev.c:4782
> 
>  netif_receive_generic_xdp+0x639/0x910 net/core/dev.c:4845
> 
>  do_xdp_generic net/core/dev.c:4904 [inline]
> 
>  __netif_receive_skb_core+0x290f/0x6360 net/core/dev.c:5310
> 
>  __netif_receive_skb_one_core net/core/dev.c:5487 [inline]
> 
>  __netif_receive_skb+0xc8/0x5d0 net/core/dev.c:5603
> 
>  process_backlog+0x45a/0x890 net/core/dev.c:5931
> 
> Since this is syzbot, the skb will have come from a tun device,
> 
> seemingly with IFF_NAPI, and maybe IFF_NAPI_FRAGS.
> 
> But relevant to bpf_xdp_adjust_head is how the xdp metadata
> 

Thanks.

I'm wondering if we can directly perform a memset in bpf_xdp_adjust_head
when users execute an expand header (offset < 0).

Although the main purpose of bpf_xdp_adjust_head is to write new headers,
it's possible that some users might be doing this to read lower-layer
headers, in which case memset would be inappropriate.

However, I found that when expanding headers, it also involves copying
data meta forward, which would overwrite padding memory, so maybe I'm
overthinking this?

In general, since bpf_xdp_adjust_head can access skb->head, it exposes a
minimum of XDP_PACKET_HEADROOM (256) uninitialized bytes to users, and
I'm not entirely clear if there are any security implications.


diff --git a/net/core/filter.c b/net/core/filter.c
index 2ec162dd83c4..51f3f0d9b4bb 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3947,6 +3947,8 @@ BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset)
        if (metalen)
                memmove(xdp->data_meta + offset,
                        xdp->data_meta, metalen);
+       if (offset < 0)
+               memset(data, 0, -offset);
        xdp->data_meta += offset;
        xdp->data = data;