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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHmME9rV1=deEkHS=W4ApHSRyN2M=VGhqcYh76DrB3ywDEce0w@mail.gmail.com>
Date:   Sat, 27 Feb 2021 01:33:25 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: always use icmp{,v6}_ndo_send from ndo_start_xmit

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

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.

> Please cc: the maintainers for patches that are meant to be merged, btw.

Whoops. I'll do so and repost.