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]

Date:   Thu, 18 Feb 2021 16:40:09 +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] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending

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.

> 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.

Jason