| Message ID | 20210410095149.3708143-1-phil@philpotter.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: core: sk_buff: zero-fill skb->data in __alloc_skb function | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: jonathan.lemon@gmail.com cong.wang@bytedance.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 7 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Sat, Apr 10, 2021 at 11:51 AM Phillip Potter <phil@philpotter.co.uk> wrote: > > Zero-fill skb->data in __alloc_skb function of net/core/skbuff.c, > up to start of struct skb_shared_info bytes. Fixes a KMSAN-found > uninit-value bug reported by syzbot at: > https://syzkaller.appspot.com/bug?id=abe95dc3e3e9667fc23b8d81f29ecad95c6f106f > > Reported-by: syzbot+2e406a9ac75bb71d4b7a@syzkaller.appspotmail.com > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > --- > net/core/skbuff.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index 785daff48030..9ac26cdb5417 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -215,6 +215,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, > * to allow max possible filling before reallocation. > */ > size = SKB_WITH_OVERHEAD(ksize(data)); > + memset(data, 0, size); > prefetchw(data + size); Certainly not. There is a difference between kmalloc() and kzalloc() Here you are basically silencing KMSAN and make it useless. Please fix the real issue, or stop using KMSAN if it bothers you.
On Sat, Apr 10, 2021 at 12:12 PM Eric Dumazet <edumazet@google.com> wrote: > > On Sat, Apr 10, 2021 at 11:51 AM Phillip Potter <phil@philpotter.co.uk> wrote: > > > > Zero-fill skb->data in __alloc_skb function of net/core/skbuff.c, > > up to start of struct skb_shared_info bytes. Fixes a KMSAN-found > > uninit-value bug reported by syzbot at: > > https://syzkaller.appspot.com/bug?id=abe95dc3e3e9667fc23b8d81f29ecad95c6f106f > > > > Reported-by: syzbot+2e406a9ac75bb71d4b7a@syzkaller.appspotmail.com > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > --- > > net/core/skbuff.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index 785daff48030..9ac26cdb5417 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -215,6 +215,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, > > * to allow max possible filling before reallocation. > > */ > > size = SKB_WITH_OVERHEAD(ksize(data)); > > + memset(data, 0, size); > > prefetchw(data + size); > > > Certainly not. > > There is a difference between kmalloc() and kzalloc() > > Here you are basically silencing KMSAN and make it useless. > > Please fix the real issue, or stop using KMSAN if it bothers you. My understanding of the KMSAN bug (when I released it months ago) was that it was triggered by some invalid assumptions in geneve_xmit() The syzbot repro sends a packet with a very small size (Ethernet header only) and no IP/IPv6 header Fix for ipv4 part (sorry, not much time during week end to test all this) diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c index e3b2375ac5eb55f544bbc1f309886cc9be189fd1..0a72779bc74bc50c20c34c05b2c525cca829f33c 100644 --- a/drivers/net/geneve.c +++ b/drivers/net/geneve.c @@ -892,6 +892,9 @@ static int geneve_xmit_skb(struct sk_buff *skb, struct net_device *dev, __be16 sport; int err; + if (!pskb_network_may_pull(skb, sizeof(struct iphdr)) + return -EINVAL; + sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true); rt = geneve_get_v4_rt(skb, dev, gs4, &fl4, info, geneve->cfg.info.key.tp_dst, sport);
On Sat, Apr 10, 2021 at 01:00:34PM +0200, Eric Dumazet wrote: > On Sat, Apr 10, 2021 at 12:12 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Sat, Apr 10, 2021 at 11:51 AM Phillip Potter <phil@philpotter.co.uk> wrote: > > > > > > Zero-fill skb->data in __alloc_skb function of net/core/skbuff.c, > > > up to start of struct skb_shared_info bytes. Fixes a KMSAN-found > > > uninit-value bug reported by syzbot at: > > > https://syzkaller.appspot.com/bug?id=abe95dc3e3e9667fc23b8d81f29ecad95c6f106f > > > > > > Reported-by: syzbot+2e406a9ac75bb71d4b7a@syzkaller.appspotmail.com > > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk> > > > --- > > > net/core/skbuff.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > > index 785daff48030..9ac26cdb5417 100644 > > > --- a/net/core/skbuff.c > > > +++ b/net/core/skbuff.c > > > @@ -215,6 +215,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, > > > * to allow max possible filling before reallocation. > > > */ > > > size = SKB_WITH_OVERHEAD(ksize(data)); > > > + memset(data, 0, size); > > > prefetchw(data + size); > > > > > > Certainly not. > > > > There is a difference between kmalloc() and kzalloc() > > > > Here you are basically silencing KMSAN and make it useless. > > > > Please fix the real issue, or stop using KMSAN if it bothers you. > > My understanding of the KMSAN bug (when I released it months ago) was > that it was triggered by some invalid assumptions in geneve_xmit() > > The syzbot repro sends a packet with a very small size (Ethernet > header only) and no IP/IPv6 header > > Fix for ipv4 part (sorry, not much time during week end to test all this) > > diff --git a/drivers/net/geneve.c b/drivers/net/geneve.c > index e3b2375ac5eb55f544bbc1f309886cc9be189fd1..0a72779bc74bc50c20c34c05b2c525cca829f33c > 100644 > --- a/drivers/net/geneve.c > +++ b/drivers/net/geneve.c > @@ -892,6 +892,9 @@ static int geneve_xmit_skb(struct sk_buff *skb, > struct net_device *dev, > __be16 sport; > int err; > > + if (!pskb_network_may_pull(skb, sizeof(struct iphdr)) > + return -EINVAL; > + > sport = udp_flow_src_port(geneve->net, skb, 1, USHRT_MAX, true); > rt = geneve_get_v4_rt(skb, dev, gs4, &fl4, info, > geneve->cfg.info.key.tp_dst, sport); Dear Eric, Thank you for your help/feedback. I have crafted a patch using your code above and the equivalent check for geneve6_xmit_skb, and this works for me on my local KMSAN build. I am also running it through syzbot to check there as well. I will send out with appropriate attribution assuming all is OK on the testing front. Regards, Phil
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index 785daff48030..9ac26cdb5417 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -215,6 +215,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, * to allow max possible filling before reallocation. */ size = SKB_WITH_OVERHEAD(ksize(data)); + memset(data, 0, size); prefetchw(data + size); /*
Zero-fill skb->data in __alloc_skb function of net/core/skbuff.c, up to start of struct skb_shared_info bytes. Fixes a KMSAN-found uninit-value bug reported by syzbot at: https://syzkaller.appspot.com/bug?id=abe95dc3e3e9667fc23b8d81f29ecad95c6f106f Reported-by: syzbot+2e406a9ac75bb71d4b7a@syzkaller.appspotmail.com Signed-off-by: Phillip Potter <phil@philpotter.co.uk> --- net/core/skbuff.c | 1 + 1 file changed, 1 insertion(+)