On Thu, Feb 18, 2021 at 10:40 AM Jason A. Donenfeld <ja...@zx2c4.com> wrote:
>
> Hi Willem,
>
> On Thu, Feb 18, 2021 at 3:57 PM Willem de Bruijn
> <willemdebruijn.ker...@gmail.com> wrote:
> >
> > On Thu, Feb 18, 2021 at 7:31 AM Jason A. Donenfeld <ja...@zx2c4.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.
