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

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

[Willem de Bruijn]

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@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/00000000000067f65105edbd295d@google.com/T/
> Signed-off-by: Jiayuan Chen <jiayuan.chen@linux.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
was setup with xdp_prepare_buff, which here is called from
bpf_prog_run_generic_xdp:

        /* The XDP program wants to see the packet starting at the MAC
         * header.
         */
        mac_len = skb->data - skb_mac_header(skb);
        hard_start = skb->data - skb_headroom(skb);

        /* SKB "head" area always have tailroom for skb_shared_info */
        frame_sz = (void *)skb_end_pointer(skb) - hard_start;
        frame_sz += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

        rxqueue = netif_get_rxqueue(skb);
        xdp_init_buff(xdp, frame_sz, &rxqueue->xdp_rxq);
        xdp_prepare_buff(xdp, hard_start, skb_headroom(skb) - mac_len,
                         skb_headlen(skb) + mac_len, true);

> @@ -1621,6 +1622,7 @@ static struct sk_buff *tun_build_skb(struct tun_struct *tun,
>  		return ERR_PTR(-ENOMEM);
>  
>  	buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> +	memset(buf, 0, pad);
>  	copied = copy_page_from_iter(alloc_frag->page,
>  				     alloc_frag->offset + pad,
>  				     len, from);
> -- 
> 2.47.1
> 

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

[Jiayuan Chen]

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

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

[Jakub Kicinski]

On Fri, 28 Mar 2025 09:15:53 +0000 Jiayuan Chen wrote:
> I'm wondering if we can directly perform a memset in bpf_xdp_adjust_head
> when users execute an expand header (offset < 0).

Same situation happens in bpf_xdp_adjust_meta(), but I'm pretty
sure this was discussed and considered too high cost for XDP.
Could you find the old discussions and double check the arguments
made back then? Opinions may have changed but let's make sure we're
not missing anything. And performance numbers would be good to have
since the main reason this isn't done today was perf.

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

[Lei Yang]

QE tested this patch with virtio-net regression tests, everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>


On Fri, Mar 28, 2025 at 7:39 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 28 Mar 2025 09:15:53 +0000 Jiayuan Chen wrote:
> > I'm wondering if we can directly perform a memset in bpf_xdp_adjust_head
> > when users execute an expand header (offset < 0).
>
> Same situation happens in bpf_xdp_adjust_meta(), but I'm pretty
> sure this was discussed and considered too high cost for XDP.
> Could you find the old discussions and double check the arguments
> made back then? Opinions may have changed but let's make sure we're
> not missing anything. And performance numbers would be good to have
> since the main reason this isn't done today was perf.
>