| Message ID | 20250327134122.399874-1-jiayuan.chen@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1] net: Fix tuntap uninitialized value | expand |
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 >
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;
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.
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. >
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; @@ -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);
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(+)