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]

Message-ID: <CA+FuTSdiuPK-V5oJOMC7fQsjQKRLt95oP7OAOtR3S5mfUJreKg@mail.gmail.com>
Date:   Thu, 18 Feb 2021 15:15:54 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     "Jason A. Donenfeld" <Jason@...c4.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 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.