linux-kernel - Re: [PATCH net v1] net: Fix tuntap uninitialized value

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <17a3bc7273fac6a2e647a6864212510b37b96ab2@linux.dev>
Date: Fri, 28 Mar 2025 09:15:53 +0000
From: "Jiayuan Chen" <jiayuan.chen@...ux.dev>
To: "Willem de Bruijn" <willemdebruijn.kernel@...il.com>,
 netdev@...r.kernel.org
Cc: willemdebruijn.kernel@...il.com, jasowang@...hat.com,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, ast@...nel.org, daniel@...earbox.net,
 hawk@...nel.org, john.fastabend@...il.com, linux-kernel@...r.kernel.org,
 syzbot+0e6ddb1ef80986bdfe64@...kaller.appspotmail.com,
 bpf@...r.kernel.org, martin.lau@...ux.dev, eddyz87@...il.com,
 song@...nel.org, kpsingh@...nel.org, jolsa@...nel.org
Subject: Re: [PATCH net v1] net: Fix tuntap uninitialized value

March 28, 2025 at 05:08, "Willem de Bruijn" <willemdebruijn.kernel@...il.com> 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@...kaller.appspotmail.com
> >  Closes: https://lore.kernel.org/all/00000000000067f65105edbd295d@google.com/T/
> >  Signed-off-by: Jiayuan Chen <jiayuan.chen@...ux.dev>
> > 
> >  ---
> > 
> >  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;