netdev - Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo

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]

Date:   Thu, 18 Feb 2021 21:39:31 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        SinYu <liuxyon@...il.com>
Subject: Re: [PATCH net v2] net: icmp: pass zeroed opts from
 icmp{,v6}_ndo_send before sending

On Thu, Feb 18, 2021 at 9:37 PM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> On Thu, Feb 18, 2021 at 3:25 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 9:16 PM Willem de Bruijn
> > <willemdebruijn.kernel@...il.com> wrote:
> > >
> > > On Thu, Feb 18, 2021 at 12:58 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> > > >
> > > > On Thu, Feb 18, 2021 at 5:34 PM Willem de Bruijn
> > > > <willemdebruijn.kernel@...il.com> wrote:
> > > > > Thanks for respinning.
> > > > >
> > > > > Making ipv4 and ipv6 more aligned is a good goal, but more for
> > > > > net-next than bug fixes that need to be backported to many stable
> > > > > branches.
> > > > >
> > > > > Beyond that, I'm not sure this fixes additional cases vs the previous
> > > > > patch? It uses new on-stack variables instead of skb->cb, which again
> > > > > is probably good in general, but adds more change than is needed for
> > > > > the stable fix.
> > > >
> > > > It doesn't appear to be problematic for applying to stable. I think
> > > > this v2 is the "right way" to handle it. Zeroing out skb->cb is
> > > > unexpected and weird anyway. What if the caller was expecting to use
> > > > their skb->cb after calling icmp_ndo_send? Did they think it'd get
> > > > wiped out like that? This v2 prevents that weird behavior from
> > > > happening.
> > > >
> > > > > My comment on fixing all callers of  icmp{,v6}_send was wrong, in
> > > > > hindsight. In most cases IPCB is set correctly before calling those,
> > > > > so we cannot just zero inside those. If we can only address the case
> > > > > for icmp{,v6}_ndo_send I think the previous patch introduced less
> > > > > churn, so is preferable. Unless I'm missing something.
> > > >
> > > > As mentioned above it's weird and unexpected.
> > > >
> > > > > Reminder of two main comments: sufficient to zero sizeof(IPCB..) and
> > > > > if respinning, please explicitly mention the path that leads to a
> > > > > stack overflow, as it is not immediately obvious (even from reading
> > > > > the fix code?).
> > > >
> > > > I don't intend to respin v1, as I think v2 is more correct, and I
> > > > don't think only zeroing IPCB is a smart idea, as in the future that
> > > > code is bound to break when somebody forgets to update it. This v2
> > > > does away with the zeroing all together, though, so that the right
> > > > bytes to be zeroed are properly enforced all the time by the type
> > > > system.
> > >
> > > I'm afraid this latest version seems to have build issues, as per the
> > > patchwork bot.
> >
> > Hmm I didn't get those bot emails. Either way, I'll do a bit of build
> > testing with different config knobs now and send a v3. Thanks for
> > letting me know.
>
> Different bot :)
>
> You might get emails from the other later. These can be found through
> patchwork at
>
> https://patchwork.kernel.org/project/netdevbpf/patch/20210218160745.2343501-1-Jason@zx2c4.com/

Wow, that is an awesome bot! Can't believe I hadn't seen that.
v3 is posted at
https://patchwork.kernel.org/project/netdevbpf/patch/20210218203404.2429186-1-Jason@zx2c4.com/
and I guess now we're waiting for the bots to get whirling on it.

Jason