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: [email protected]
Signed-off-by: Phillip Potter <[email protected]>
---
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);
/*
--
2.30.2
On Sat, Apr 10, 2021 at 11:51 AM Phillip Potter <[email protected]> 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: [email protected]
> Signed-off-by: Phillip Potter <[email protected]>
> ---
> 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 <[email protected]> wrote:
>
> On Sat, Apr 10, 2021 at 11:51 AM Phillip Potter <[email protected]> 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: [email protected]
> > Signed-off-by: Phillip Potter <[email protected]>
> > ---
> > 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 <[email protected]> wrote:
> >
> > On Sat, Apr 10, 2021 at 11:51 AM Phillip Potter <[email protected]> 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: [email protected]
> > > Signed-off-by: Phillip Potter <[email protected]>
> > > ---
> > > 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