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 [off-list ref] wrote:

Hi Willem,

On Thu, Feb 18, 2021 at 3:57 PM Willem de Bruijn
[off-list ref] wrote:

quoted

On Thu, Feb 18, 2021 at 7:31 AM Jason A. Donenfeld [off-list ref] wrote:

quoted

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.

quoted

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.