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:24:54 +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: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.

Jason