Re: [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending

On Thu, Feb 18, 2021 at 8:08 PM Jakub Kicinski [off-list ref] wrote:

On Thu, 18 Feb 2021 13:30:53 +0100 Jason A. Donenfeld wrote:

quoted

The icmp{,v6}_send functions make all sorts of use of skb->cb, assuming
the skb to have come directly from the inet layer. But when the packet
comes from the ndo layer, especially when forwarded, there's no telling
what might be in skb->cb at that point. So, icmp{,v6}_ndo_send must zero
out its skb->cb before passing the packet off to icmp{,v6}_send.
Otherwise the icmp sending code risks reading bogus memory contents,
which can result in nasty stack overflows such as this one reported by a
user:

    panic+0x108/0x2ea
    __stack_chk_fail+0x14/0x20
    __icmp_send+0x5bd/0x5c0
    icmp_ndo_send+0x148/0x160

This is easy to simulate by doing a `memset(skb->cb, 0x41,
sizeof(skb->cb));` before calling icmp{,v6}_ndo_send, and it's only by
good fortune and the rarity of icmp sending from that context that we've
avoided reports like this until now. For example, in KASAN:

    BUG: KASAN: stack-out-of-bounds in __ip_options_echo+0xa0e/0x12b0
    Write of size 38 at addr ffff888006f1f80e by task ping/89
    CPU: 2 PID: 89 Comm: ping Not tainted 5.10.0-rc7-debug+ #5
    Call Trace:
     dump_stack+0x9a/0xcc
     print_address_description.constprop.0+0x1a/0x160
     __kasan_report.cold+0x20/0x38
     kasan_report+0x32/0x40
     check_memory_region+0x145/0x1a0
     memcpy+0x39/0x60
     __ip_options_echo+0xa0e/0x12b0
     __icmp_send+0x744/0x1700

Actually, out of the 4 drivers that do this, only gtp zeroed the cb for
the v4 case, while the rest did not. So this commit actually removes the
gtp-specific zeroing, while putting the code where it belongs in the
shared infrastructure of icmp{,v6}_ndo_send.

Fixes: a2b78e9b2cac ("sunvnet: generate ICMP PTMUD messages for smaller port MTUs")

nit: please make sure you CC the authors of the commits you're blaming.

Will do. Though in this case, it's behavior that's a few places, so I
put the git commit of the earliest case, to aid with backporting.

(This email is a reply to v1, but please check out v2.)

Jason