netdev - Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for

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: <CANpmjNNbK=99yjoWFOmPGHM8BH7U44v9qAyo6ZbC+Vap58iPPQ@mail.gmail.com>
Date:   Mon, 1 Feb 2021 18:33:56 +0100
From:   Marco Elver <elver@...gle.com>
To:     Christoph Paasch <christoph.paasch@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        kasan-dev <kasan-dev@...glegroups.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Willem de Bruijn <willemb@...gle.com>, linmiaohe@...wei.com,
        gnault@...hat.com, dseok.yi@...sung.com, kyk.segfault@...il.com,
        Al Viro <viro@...iv.linux.org.uk>,
        netdev <netdev@...r.kernel.org>,
        Alexander Potapenko <glider@...gle.com>,
        syzbot <syzbot+7b99aafdcc2eedea6178@...kaller.appspotmail.com>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net-next] net: fix up truesize of cloned skb in skb_prepare_for_shift()

On Mon, 1 Feb 2021 at 17:50, Christoph Paasch
<christoph.paasch@...il.com> wrote:
> On Mon, Feb 1, 2021 at 8:09 AM Marco Elver <elver@...gle.com> wrote:
> >
> > Avoid the assumption that ksize(kmalloc(S)) == ksize(kmalloc(S)): when
> > cloning an skb, save and restore truesize after pskb_expand_head(). This
> > can occur if the allocator decides to service an allocation of the same
> > size differently (e.g. use a different size class, or pass the
> > allocation on to KFENCE).
> >
> > Because truesize is used for bookkeeping (such as sk_wmem_queued), a
> > modified truesize of a cloned skb may result in corrupt bookkeeping and
> > relevant warnings (such as in sk_stream_kill_queues()).
> >
> > Link: https://lkml.kernel.org/r/X9JR/J6dMMOy1obu@elver.google.com
> > Reported-by: syzbot+7b99aafdcc2eedea6178@...kaller.appspotmail.com
> > Suggested-by: Eric Dumazet <edumazet@...gle.com>
> > Signed-off-by: Marco Elver <elver@...gle.com>
> > ---
> >  net/core/skbuff.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 2af12f7e170c..3787093239f5 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -3289,7 +3289,19 @@ EXPORT_SYMBOL(skb_split);
> >   */
> >  static int skb_prepare_for_shift(struct sk_buff *skb)
> >  {
> > -       return skb_cloned(skb) && pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> > +       int ret = 0;
> > +
> > +       if (skb_cloned(skb)) {
> > +               /* Save and restore truesize: pskb_expand_head() may reallocate
> > +                * memory where ksize(kmalloc(S)) != ksize(kmalloc(S)), but we
> > +                * cannot change truesize at this point.
> > +                */
> > +               unsigned int save_truesize = skb->truesize;
> > +
> > +               ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
> > +               skb->truesize = save_truesize;
> > +       }
> > +       return ret;
>
> just a few days ago we found out that this also fixes a syzkaller
> issue on MPTCP (https://github.com/multipath-tcp/mptcp_net-next/issues/136).
> I confirmed that this patch fixes the issue for us as well:
>
> Tested-by: Christoph Paasch <christoph.paasch@...il.com>

That's interesting, because according to your config you did not have
KFENCE enabled. Although it's hard to say what exactly caused the
truesize mismatch in your case, because it clearly can't be KFENCE
that caused ksize(kmalloc(S))!=ksize(kmalloc(S)) for you.

Thanks,
-- Marco