netdev - Re: [PATCH] net: always use icmp{,v6}_ndo_send from ndo_start

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 26 Feb 2021 21:10:03 -0500
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: always use icmp{,v6}_ndo_send from ndo_start_xmit

On Fri, Feb 26, 2021 at 7:42 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
>
> On Sat, Feb 27, 2021 at 12:29 AM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > On Fri, Feb 26, 2021 at 5:39 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> > >
> > > On Fri, Feb 26, 2021 at 10:25 PM Willem de Bruijn
> > > <willemdebruijn.kernel@...il.com> wrote:
> > > >
> > > > On Thu, Feb 25, 2021 at 6:46 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> > > > >
> > > > > There were a few remaining tunnel drivers that didn't receive the prior
> > > > > conversion to icmp{,v6}_ndo_send. Knowing now that this could lead to
> > > > > memory corrution (see ee576c47db60 ("net: icmp: pass zeroed opts from
> > > > > icmp{,v6}_ndo_send before sending") for details), there's even more
> > > > > imperative to have these all converted. So this commit goes through the
> > > > > remaining cases that I could find and does a boring translation to the
> > > > > ndo variety.
> > > > >
> > > > > Cc: Willem de Bruijn <willemb@...gle.com>
> > > > > Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
> > > >
> > > > Using a stack variable over skb->cb[] is definitely the right fix for
> > > > all of these. Thanks Jason.
> > > >
> > > > Only part that I don't fully know is the conntrack conversion. That is
> > > > a behavioral change. What is the context behind that? I assume it's
> > > > fine. In that if needed, that is the case for all devices, nothing
> > > > specific about the couple that call icmp(v6)_ndo_send already.
> > >
> > > That's actually a sensible change anyway. icmp_send does something
> > > bogus if the packet has already passed through netfilter, which is why
> > > the ndo variant was adopted. So it's good and correct for these to
> > > change in that way.
> > >
> > > Jason
> >
> > Something bogus, how? Does this apply to all uses of conntrack?
> > Specifically NAT? Not trying to be obtuse, but I really find it hard
> > to evaluate that part.
>
> By the time packets hit ndo_start_xmit, the src address has changed,
> and icmp can't deliver to the actual source, and its rate limiting
> works against the wrong source. All of this was explained, justified,
> and discussed on the original icmp_ndo_start patchset, which included
> the function and converted drivers to use it. However, a few spots
> were missed, which this patchset cleans up. Here's the merge with
> details:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=803381f9f117493d6204d82445a530c834040fe6

Thanks for the link. I used git blame to a look at the patch that
introduced the code before asking for more context, but this state was
in the merge commit, so I missed it.

> The network devices that this patch here adjusts are no different than
> the four I originally fixed up in that series -- xfrmi, gtp, sunvnet,
> wireguard.

Agreed.


> > Please cc: the maintainers for patches that are meant to be merged, btw.
>
> Whoops. I'll do so and repost.