netdev - Re: [PATCH net] net: icmp: zero-out cb in 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: <CAF=yD-Kw=gmPA1qRY0zEVdUQue35L+Y99JySxyG2--LubvptWQ@mail.gmail.com>
Date:   Thu, 18 Feb 2021 12:29:45 -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] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending

On Thu, Feb 18, 2021 at 10:40 AM Jason A. Donenfeld <Jason@...c4.com> wrote:
>
> Hi Willem,
>
> On Thu, Feb 18, 2021 at 3:57 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 7:31 AM Jason A. Donenfeld <Jason@...c4.com> wrote:
> > >
> > > The icmp{,v6}_send functions make all sorts of use of skb->cb, assuming
> >
> > Indeed that also casts skb->cb, to read IP6CB(skb)->iif, good catch.
> >
> > Still, might be good to more precisely detail the relevant bug:
> > icmp_send casts the cb to an option struct.
> >
> >         __icmp_send(skb_in, type, code, info, &IPCB(skb_in)->opt);
> >
> > which is referenced to parse headers by __ip_options_echo, copying
> > data into stack allocated icmp_param and so overwriting the stack
> > frame.
>
> The other way to fix this bug would be to just make icmp_ndo_send call
> __icmp_send with an zeored stack-allocated ip_options, rather than
> calling icmp_send which calls __icmp_send with the IPCB one. The
> implementation of this is very easy, and that's what I did at first,
> until I noticed that the v6 side would require a little bit more
> plumbing to do right. But, I can go ahead and do that, if you think
> that's the better strategy.

Thanks for that. It does seem to add more code change that we'd like
for stable backports.

> > This is from looking at all the callers of icmp{,v6}_ndo_send.
> >
> > If you look at the callers of icmp{,v6}_send there are even a couple
> > more. Such as ipoib_cm_skb_reap (which memsets), clip_neigh_error
> > (which doesn't), various tunnel devices (which live under net/ipv4,
> > but are called as .ndo_start_xmit downstream from, e.g., segmentation
> > (SKB_GSO_CB). Which are fixed (all?) in commit 5146d1f15112
> > ("tunnel: Clear IPCB(skb)->opt before dst_link_failure called").
> >
> > Might be even better to do the memset in __icmp_send/icmp6_send,
> > rather than in the wrapper. What do you think?
>
> I don't think memsetting from icmp_send itself is a good idea, since
> most callers of that are actually from the inet layer, where it makes
> sense to look at IPCB. Other callers, from the ndo layer, should be
> using the icmp_ndo_send helper instead. Or am I confused?
>
> If there are places that are using icmp_send from ndo_start_xmit,
> that's a problem that should be fixed, with those uses swapped for
> icmp_ndo_send.

I missed this response earlier (two inboxes). Agreed. Sorry that I
didn't reply before your v2.