* [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending @ 2021-02-18 12:30 Jason A. Donenfeld 2021-02-18 14:56 ` Willem de Bruijn 2021-02-18 19:08 ` Jakub Kicinski 0 siblings, 2 replies; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-18 12:30 UTC (permalink / raw) To: netdev; +Cc: Jason A. Donenfeld, SinYu, Willem de Bruijn 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") Reported-by: SinYu <liuxyon@gmail.com> Cc: Willem de Bruijn <willemb@google.com> Link: https://lore.kernel.org/netdev/CAF=yD-LOF116aHub6RMe8vB8ZpnrrnoTdqhobEx+bvoA8AsP0w@mail.gmail.com/T/ Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/net/gtp.c | 1 - include/linux/icmpv6.h | 6 +++++- include/net/icmp.h | 6 +++++- net/ipv4/icmp.c | 2 ++ net/ipv6/ip6_icmp.c | 2 ++ 5 files changed, 14 insertions(+), 3 deletions(-) diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index 4c04e271f184..fd3c2d86e48b 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -539,7 +539,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, if (!skb_is_gso(skb) && (iph->frag_off & htons(IP_DF)) && mtu < ntohs(iph->tot_len)) { netdev_dbg(dev, "packet too big, fragmentation needed\n"); - memset(IPCB(skb), 0, sizeof(*IPCB(skb))); icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED, htonl(mtu)); goto err_rt; diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h index 1b3371ae8193..87d434fc98a3 100644 --- a/include/linux/icmpv6.h +++ b/include/linux/icmpv6.h @@ -45,7 +45,11 @@ int ip6_err_gen_icmpv6_unreach(struct sk_buff *skb, int nhs, int type, #if IS_ENABLED(CONFIG_NF_NAT) void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info); #else -#define icmpv6_ndo_send icmpv6_send +static inline void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) +{ + memset(skb_in->cb, 0, sizeof(skb_in->cb)); + icmpv6_send(skb_in, type, code, info); +} #endif #else diff --git a/include/net/icmp.h b/include/net/icmp.h index 9ac2d2672a93..4bb404c9abc8 100644 --- a/include/net/icmp.h +++ b/include/net/icmp.h @@ -46,7 +46,11 @@ static inline void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 #if IS_ENABLED(CONFIG_NF_NAT) void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info); #else -#define icmp_ndo_send icmp_send +static inline void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) +{ + memset(skb_in->cb, 0, sizeof(skb_in->cb)); + icmp_send(skb_in, type, code, info); +} #endif int icmp_rcv(struct sk_buff *skb); diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index 396b492c804f..ecf080532291 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -781,6 +781,7 @@ void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) ct = nf_ct_get(skb_in, &ctinfo); if (!ct || !(ct->status & IPS_SRC_NAT)) { + memset(skb_in->cb, 0, sizeof(skb_in->cb)); icmp_send(skb_in, type, code, info); return; } @@ -796,6 +797,7 @@ void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) orig_ip = ip_hdr(skb_in)->saddr; ip_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.ip; + memset(skb_in->cb, 0, sizeof(skb_in->cb)); icmp_send(skb_in, type, code, info); ip_hdr(skb_in)->saddr = orig_ip; out: diff --git a/net/ipv6/ip6_icmp.c b/net/ipv6/ip6_icmp.c index 70c8c2f36c98..ddc28be8a65d 100644 --- a/net/ipv6/ip6_icmp.c +++ b/net/ipv6/ip6_icmp.c @@ -57,6 +57,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) ct = nf_ct_get(skb_in, &ctinfo); if (!ct || !(ct->status & IPS_SRC_NAT)) { + memset(skb_in->cb, 0, sizeof(skb_in->cb)); icmpv6_send(skb_in, type, code, info); return; } @@ -72,6 +73,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) orig_ip = ipv6_hdr(skb_in)->saddr; ipv6_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.in6; + memset(skb_in->cb, 0, sizeof(skb_in->cb)); icmpv6_send(skb_in, type, code, info); ipv6_hdr(skb_in)->saddr = orig_ip; out: -- 2.30.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending 2021-02-18 12:30 [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending Jason A. Donenfeld @ 2021-02-18 14:56 ` Willem de Bruijn 2021-02-18 15:40 ` Jason A. Donenfeld 2021-02-18 19:08 ` Jakub Kicinski 1 sibling, 1 reply; 16+ messages in thread From: Willem de Bruijn @ 2021-02-18 14:56 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: Network Development, SinYu On Thu, Feb 18, 2021 at 7:31 AM Jason A. Donenfeld <Jason@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 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") 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? > Reported-by: SinYu <liuxyon@gmail.com> > Cc: Willem de Bruijn <willemb@google.com> > Link: https://lore.kernel.org/netdev/CAF=yD-LOF116aHub6RMe8vB8ZpnrrnoTdqhobEx+bvoA8AsP0w@mail.gmail.com/T/ > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > drivers/net/gtp.c | 1 - > include/linux/icmpv6.h | 6 +++++- > include/net/icmp.h | 6 +++++- > net/ipv4/icmp.c | 2 ++ > net/ipv6/ip6_icmp.c | 2 ++ > 5 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c > index 4c04e271f184..fd3c2d86e48b 100644 > --- a/drivers/net/gtp.c > +++ b/drivers/net/gtp.c > @@ -539,7 +539,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, > if (!skb_is_gso(skb) && (iph->frag_off & htons(IP_DF)) && > mtu < ntohs(iph->tot_len)) { > netdev_dbg(dev, "packet too big, fragmentation needed\n"); > - memset(IPCB(skb), 0, sizeof(*IPCB(skb))); > icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED, > htonl(mtu)); > goto err_rt; > diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h > index 1b3371ae8193..87d434fc98a3 100644 > --- a/include/linux/icmpv6.h > +++ b/include/linux/icmpv6.h > @@ -45,7 +45,11 @@ int ip6_err_gen_icmpv6_unreach(struct sk_buff *skb, int nhs, int type, > #if IS_ENABLED(CONFIG_NF_NAT) > void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info); > #else > -#define icmpv6_ndo_send icmpv6_send > +static inline void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > +{ > + memset(skb_in->cb, 0, sizeof(skb_in->cb)); > + icmpv6_send(skb_in, type, code, info); > +} > #endif > > #else > diff --git a/include/net/icmp.h b/include/net/icmp.h > index 9ac2d2672a93..4bb404c9abc8 100644 > --- a/include/net/icmp.h > +++ b/include/net/icmp.h > @@ -46,7 +46,11 @@ static inline void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 > #if IS_ENABLED(CONFIG_NF_NAT) > void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info); > #else > -#define icmp_ndo_send icmp_send > +static inline void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) > +{ > + memset(skb_in->cb, 0, sizeof(skb_in->cb)); as in the line removed in gtp_build_skb_ip4, it would be sufficient to memset sizeof(*IPCB(skb)). I don't know if you chose to clear the full 40B on purpose. > + icmp_send(skb_in, type, code, info); > +} > #endif > > int icmp_rcv(struct sk_buff *skb); > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > index 396b492c804f..ecf080532291 100644 > --- a/net/ipv4/icmp.c > +++ b/net/ipv4/icmp.c > @@ -781,6 +781,7 @@ void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) > > ct = nf_ct_get(skb_in, &ctinfo); > if (!ct || !(ct->status & IPS_SRC_NAT)) { > + memset(skb_in->cb, 0, sizeof(skb_in->cb)); > icmp_send(skb_in, type, code, info); > return; > } > @@ -796,6 +797,7 @@ void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) > > orig_ip = ip_hdr(skb_in)->saddr; > ip_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.ip; > + memset(skb_in->cb, 0, sizeof(skb_in->cb)); > icmp_send(skb_in, type, code, info); > ip_hdr(skb_in)->saddr = orig_ip; > out: > diff --git a/net/ipv6/ip6_icmp.c b/net/ipv6/ip6_icmp.c > index 70c8c2f36c98..ddc28be8a65d 100644 > --- a/net/ipv6/ip6_icmp.c > +++ b/net/ipv6/ip6_icmp.c > @@ -57,6 +57,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > > ct = nf_ct_get(skb_in, &ctinfo); > if (!ct || !(ct->status & IPS_SRC_NAT)) { > + memset(skb_in->cb, 0, sizeof(skb_in->cb)); > icmpv6_send(skb_in, type, code, info); > return; > } > @@ -72,6 +73,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > > orig_ip = ipv6_hdr(skb_in)->saddr; > ipv6_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.in6; > + memset(skb_in->cb, 0, sizeof(skb_in->cb)); > icmpv6_send(skb_in, type, code, info); > ipv6_hdr(skb_in)->saddr = orig_ip; > out: > -- > 2.30.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending 2021-02-18 14:56 ` Willem de Bruijn @ 2021-02-18 15:40 ` Jason A. Donenfeld 2021-02-18 16:07 ` [PATCH net v2] net: icmp: pass zeroed opts from " Jason A. Donenfeld 2021-02-18 17:29 ` [PATCH net] net: icmp: zero-out cb in " Willem de Bruijn 0 siblings, 2 replies; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-18 15:40 UTC (permalink / raw) To: Willem de Bruijn; +Cc: Network Development, SinYu Hi Willem, On Thu, Feb 18, 2021 at 3:57 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Thu, Feb 18, 2021 at 7:31 AM Jason A. Donenfeld <Jason@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. > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 15:40 ` Jason A. Donenfeld @ 2021-02-18 16:07 ` Jason A. Donenfeld 2021-02-18 16:33 ` Willem de Bruijn 2021-02-18 17:29 ` [PATCH net] net: icmp: zero-out cb in " Willem de Bruijn 1 sibling, 1 reply; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-18 16:07 UTC (permalink / raw) To: netdev; +Cc: Jason A. Donenfeld, SinYu, Willem de Bruijn The icmp{,v6}_send functions make all sorts of use of skb->cb, casting it with IPCB or IP6CB, 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. As a result, 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. This commit fixes the issue by passing an empty IPCB or IP6CB along to the functions that actually do the work. For the icmp_send, this was already trivial, thanks to __icmp_send providing the plumbing function. For icmpv6_send, this required a tiny bit of refactoring to make it behave like the v4 case, after which it was straight forward. Fixes: a2b78e9b2cac ("sunvnet: generate ICMP PTMUD messages for smaller port MTUs") Reported-by: SinYu <liuxyon@gmail.com> Cc: Willem de Bruijn <willemb@google.com> Link: https://lore.kernel.org/netdev/CAF=yD-LOF116aHub6RMe8vB8ZpnrrnoTdqhobEx+bvoA8AsP0w@mail.gmail.com/T/ Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- drivers/net/gtp.c | 1 - include/linux/icmpv6.h | 26 ++++++++++++++++++++------ include/linux/ipv6.h | 1 - include/net/icmp.h | 6 +++++- net/ipv4/icmp.c | 5 +++-- net/ipv6/icmp.c | 18 +++++++++--------- net/ipv6/ip6_icmp.c | 7 ++++--- 7 files changed, 41 insertions(+), 23 deletions(-) diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index 4c04e271f184..fd3c2d86e48b 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -539,7 +539,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, if (!skb_is_gso(skb) && (iph->frag_off & htons(IP_DF)) && mtu < ntohs(iph->tot_len)) { netdev_dbg(dev, "packet too big, fragmentation needed\n"); - memset(IPCB(skb), 0, sizeof(*IPCB(skb))); icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED, htonl(mtu)); goto err_rt; diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h index 1b3371ae8193..0a383202dd5e 100644 --- a/include/linux/icmpv6.h +++ b/include/linux/icmpv6.h @@ -3,6 +3,7 @@ #define _LINUX_ICMPV6_H #include <linux/skbuff.h> +#include <linux/ipv6.h> #include <uapi/linux/icmpv6.h> static inline struct icmp6hdr *icmp6_hdr(const struct sk_buff *skb) @@ -15,13 +16,16 @@ static inline struct icmp6hdr *icmp6_hdr(const struct sk_buff *skb) #if IS_ENABLED(CONFIG_IPV6) typedef void ip6_icmp_send_t(struct sk_buff *skb, u8 type, u8 code, __u32 info, - const struct in6_addr *force_saddr); + const struct in6_addr *force_saddr, + const struct inet6_skb_parm *parm); #if IS_BUILTIN(CONFIG_IPV6) void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, - const struct in6_addr *force_saddr); -static inline void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) + const struct in6_addr *force_saddr, + const struct inet6_skb_parm *parm); +static inline void __icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, + const struct inet6_skb_parm *parm) { - icmp6_send(skb, type, code, info, NULL); + icmp6_send(skb, type, code, info, NULL, parm); } static inline int inet6_register_icmp_sender(ip6_icmp_send_t *fn) { @@ -34,18 +38,28 @@ static inline int inet6_unregister_icmp_sender(ip6_icmp_send_t *fn) return 0; } #else -extern void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info); +extern void __icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, + const struct inet6_skb_parm *parm); extern int inet6_register_icmp_sender(ip6_icmp_send_t *fn); extern int inet6_unregister_icmp_sender(ip6_icmp_send_t *fn); #endif +static inline void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) +{ + __icmpv6_send(skb, type, code, info, IP6CB(skb)); +} + int ip6_err_gen_icmpv6_unreach(struct sk_buff *skb, int nhs, int type, unsigned int data_len); #if IS_ENABLED(CONFIG_NF_NAT) void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info); #else -#define icmpv6_ndo_send icmpv6_send +static inline void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) +{ + struct inet6_skb_parm parm = { 0 }; + __icmpv6_send(skb_in, type, code, info, &parm); +} #endif #else diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h index dda61d150a13..f514a7dd8c9c 100644 --- a/include/linux/ipv6.h +++ b/include/linux/ipv6.h @@ -84,7 +84,6 @@ struct ipv6_params { __s32 autoconf; }; extern struct ipv6_params ipv6_defaults; -#include <linux/icmpv6.h> #include <linux/tcp.h> #include <linux/udp.h> diff --git a/include/net/icmp.h b/include/net/icmp.h index 9ac2d2672a93..fd84adc47963 100644 --- a/include/net/icmp.h +++ b/include/net/icmp.h @@ -46,7 +46,11 @@ static inline void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 #if IS_ENABLED(CONFIG_NF_NAT) void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info); #else -#define icmp_ndo_send icmp_send +static inline void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) +{ + struct ip_options opts = { 0 }; + __icmp_send(skb_in, type, code, info, &opts); +} #endif int icmp_rcv(struct sk_buff *skb); diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index 396b492c804f..616e2dc1c8fa 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -775,13 +775,14 @@ EXPORT_SYMBOL(__icmp_send); void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) { struct sk_buff *cloned_skb = NULL; + struct ip_options opts = { 0 }; enum ip_conntrack_info ctinfo; struct nf_conn *ct; __be32 orig_ip; ct = nf_ct_get(skb_in, &ctinfo); if (!ct || !(ct->status & IPS_SRC_NAT)) { - icmp_send(skb_in, type, code, info); + __icmp_send(skb_in, type, code, info, &opts); return; } @@ -796,7 +797,7 @@ void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) orig_ip = ip_hdr(skb_in)->saddr; ip_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.ip; - icmp_send(skb_in, type, code, info); + __icmp_send(skb_in, type, code, info, &opts); ip_hdr(skb_in)->saddr = orig_ip; out: consume_skb(cloned_skb); diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c index f3d05866692e..fd1f896115c1 100644 --- a/net/ipv6/icmp.c +++ b/net/ipv6/icmp.c @@ -331,10 +331,9 @@ static int icmpv6_getfrag(void *from, char *to, int offset, int len, int odd, st } #if IS_ENABLED(CONFIG_IPV6_MIP6) -static void mip6_addr_swap(struct sk_buff *skb) +static void mip6_addr_swap(struct sk_buff *skb, const struct inet6_skb_parm *opt) { struct ipv6hdr *iph = ipv6_hdr(skb); - struct inet6_skb_parm *opt = IP6CB(skb); struct ipv6_destopt_hao *hao; struct in6_addr tmp; int off; @@ -351,7 +350,7 @@ static void mip6_addr_swap(struct sk_buff *skb) } } #else -static inline void mip6_addr_swap(struct sk_buff *skb) {} +static inline void mip6_addr_swap(struct sk_buff *skb, const struct inet6_skb_parm *opt) {} #endif static struct dst_entry *icmpv6_route_lookup(struct net *net, @@ -446,7 +445,8 @@ static int icmp6_iif(const struct sk_buff *skb) * Send an ICMP message in response to a packet in error */ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, - const struct in6_addr *force_saddr) + const struct in6_addr *force_saddr, + const struct inet6_skb_parm *parm) { struct inet6_dev *idev = NULL; struct ipv6hdr *hdr = ipv6_hdr(skb); @@ -542,7 +542,7 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, if (!(skb->dev->flags & IFF_LOOPBACK) && !icmpv6_global_allow(net, type)) goto out_bh_enable; - mip6_addr_swap(skb); + mip6_addr_swap(skb, parm); sk = icmpv6_xmit_lock(net); if (!sk) @@ -559,7 +559,7 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, /* select a more meaningful saddr from input if */ struct net_device *in_netdev; - in_netdev = dev_get_by_index(net, IP6CB(skb)->iif); + in_netdev = dev_get_by_index(net, parm->iif); if (in_netdev) { ipv6_dev_get_saddr(net, in_netdev, &fl6.daddr, inet6_sk(sk)->srcprefs, @@ -640,7 +640,7 @@ EXPORT_SYMBOL(icmp6_send); */ void icmpv6_param_prob(struct sk_buff *skb, u8 code, int pos) { - icmp6_send(skb, ICMPV6_PARAMPROB, code, pos, NULL); + icmp6_send(skb, ICMPV6_PARAMPROB, code, pos, NULL, IP6CB(skb)); kfree_skb(skb); } @@ -697,10 +697,10 @@ int ip6_err_gen_icmpv6_unreach(struct sk_buff *skb, int nhs, int type, } if (type == ICMP_TIME_EXCEEDED) icmp6_send(skb2, ICMPV6_TIME_EXCEED, ICMPV6_EXC_HOPLIMIT, - info, &temp_saddr); + info, &temp_saddr, IP6CB(skb2)); else icmp6_send(skb2, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, - info, &temp_saddr); + info, &temp_saddr, IP6CB(skb2)); if (rt) ip6_rt_put(rt); diff --git a/net/ipv6/ip6_icmp.c b/net/ipv6/ip6_icmp.c index 70c8c2f36c98..5f834ebc09b0 100644 --- a/net/ipv6/ip6_icmp.c +++ b/net/ipv6/ip6_icmp.c @@ -40,7 +40,7 @@ void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) rcu_read_lock(); send = rcu_dereference(ip6_icmp_send); if (send) - send(skb, type, code, info, NULL); + send(skb, type, code, info, NULL, IP6CB(skb)); rcu_read_unlock(); } EXPORT_SYMBOL(icmpv6_send); @@ -50,6 +50,7 @@ EXPORT_SYMBOL(icmpv6_send); #include <net/netfilter/nf_conntrack.h> void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) { + struct inet6_skb_parm parm = { 0 }; struct sk_buff *cloned_skb = NULL; enum ip_conntrack_info ctinfo; struct in6_addr orig_ip; @@ -57,7 +58,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) ct = nf_ct_get(skb_in, &ctinfo); if (!ct || !(ct->status & IPS_SRC_NAT)) { - icmpv6_send(skb_in, type, code, info); + __icmpv6_send(skb_in, type, code, info, &parm); return; } @@ -72,7 +73,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) orig_ip = ipv6_hdr(skb_in)->saddr; ipv6_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.in6; - icmpv6_send(skb_in, type, code, info); + __icmpv6_send(skb_in, type, code, info, &parm); ipv6_hdr(skb_in)->saddr = orig_ip; out: consume_skb(cloned_skb); -- 2.30.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 16:07 ` [PATCH net v2] net: icmp: pass zeroed opts from " Jason A. Donenfeld @ 2021-02-18 16:33 ` Willem de Bruijn 2021-02-18 17:35 ` Jason A. Donenfeld 0 siblings, 1 reply; 16+ messages in thread From: Willem de Bruijn @ 2021-02-18 16:33 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: Network Development, SinYu On Thu, Feb 18, 2021 at 11:08 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > The icmp{,v6}_send functions make all sorts of use of skb->cb, casting > it with IPCB or IP6CB, 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. As a result, 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. > > This commit fixes the issue by passing an empty IPCB or IP6CB along to > the functions that actually do the work. For the icmp_send, this was > already trivial, thanks to __icmp_send providing the plumbing function. > For icmpv6_send, this required a tiny bit of refactoring to make it > behave like the v4 case, after which it was straight forward. > > Fixes: a2b78e9b2cac ("sunvnet: generate ICMP PTMUD messages for smaller port MTUs") > Reported-by: SinYu <liuxyon@gmail.com> > Cc: Willem de Bruijn <willemb@google.com> > Link: https://lore.kernel.org/netdev/CAF=yD-LOF116aHub6RMe8vB8ZpnrrnoTdqhobEx+bvoA8AsP0w@mail.gmail.com/T/ > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> Thanks for respinning. Making ipv4 and ipv6 more aligned is a good goal, but more for net-next than bug fixes that need to be backported to many stable branches. Beyond that, I'm not sure this fixes additional cases vs the previous patch? It uses new on-stack variables instead of skb->cb, which again is probably good in general, but adds more change than is needed for the stable fix. My comment on fixing all callers of icmp{,v6}_send was wrong, in hindsight. In most cases IPCB is set correctly before calling those, so we cannot just zero inside those. If we can only address the case for icmp{,v6}_ndo_send I think the previous patch introduced less churn, so is preferable. Unless I'm missing something. Reminder of two main comments: sufficient to zero sizeof(IPCB..) and if respinning, please explicitly mention the path that leads to a stack overflow, as it is not immediately obvious (even from reading the fix code?). > --- > drivers/net/gtp.c | 1 - > include/linux/icmpv6.h | 26 ++++++++++++++++++++------ > include/linux/ipv6.h | 1 - > include/net/icmp.h | 6 +++++- > net/ipv4/icmp.c | 5 +++-- > net/ipv6/icmp.c | 18 +++++++++--------- > net/ipv6/ip6_icmp.c | 7 ++++--- > 7 files changed, 41 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c > index 4c04e271f184..fd3c2d86e48b 100644 > --- a/drivers/net/gtp.c > +++ b/drivers/net/gtp.c > @@ -539,7 +539,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, > if (!skb_is_gso(skb) && (iph->frag_off & htons(IP_DF)) && > mtu < ntohs(iph->tot_len)) { > netdev_dbg(dev, "packet too big, fragmentation needed\n"); > - memset(IPCB(skb), 0, sizeof(*IPCB(skb))); > icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED, > htonl(mtu)); > goto err_rt; > diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h > index 1b3371ae8193..0a383202dd5e 100644 > --- a/include/linux/icmpv6.h > +++ b/include/linux/icmpv6.h > @@ -3,6 +3,7 @@ > #define _LINUX_ICMPV6_H > > #include <linux/skbuff.h> > +#include <linux/ipv6.h> > #include <uapi/linux/icmpv6.h> > > static inline struct icmp6hdr *icmp6_hdr(const struct sk_buff *skb) > @@ -15,13 +16,16 @@ static inline struct icmp6hdr *icmp6_hdr(const struct sk_buff *skb) > #if IS_ENABLED(CONFIG_IPV6) > > typedef void ip6_icmp_send_t(struct sk_buff *skb, u8 type, u8 code, __u32 info, > - const struct in6_addr *force_saddr); > + const struct in6_addr *force_saddr, > + const struct inet6_skb_parm *parm); > #if IS_BUILTIN(CONFIG_IPV6) > void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > - const struct in6_addr *force_saddr); > -static inline void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) > + const struct in6_addr *force_saddr, > + const struct inet6_skb_parm *parm); > +static inline void __icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > + const struct inet6_skb_parm *parm) > { > - icmp6_send(skb, type, code, info, NULL); > + icmp6_send(skb, type, code, info, NULL, parm); > } > static inline int inet6_register_icmp_sender(ip6_icmp_send_t *fn) > { > @@ -34,18 +38,28 @@ static inline int inet6_unregister_icmp_sender(ip6_icmp_send_t *fn) > return 0; > } > #else > -extern void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info); > +extern void __icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > + const struct inet6_skb_parm *parm); > extern int inet6_register_icmp_sender(ip6_icmp_send_t *fn); > extern int inet6_unregister_icmp_sender(ip6_icmp_send_t *fn); > #endif > > +static inline void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) > +{ > + __icmpv6_send(skb, type, code, info, IP6CB(skb)); > +} > + > int ip6_err_gen_icmpv6_unreach(struct sk_buff *skb, int nhs, int type, > unsigned int data_len); > > #if IS_ENABLED(CONFIG_NF_NAT) > void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info); > #else > -#define icmpv6_ndo_send icmpv6_send > +static inline void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > +{ > + struct inet6_skb_parm parm = { 0 }; > + __icmpv6_send(skb_in, type, code, info, &parm); > +} > #endif > > #else > diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h > index dda61d150a13..f514a7dd8c9c 100644 > --- a/include/linux/ipv6.h > +++ b/include/linux/ipv6.h > @@ -84,7 +84,6 @@ struct ipv6_params { > __s32 autoconf; > }; > extern struct ipv6_params ipv6_defaults; > -#include <linux/icmpv6.h> > #include <linux/tcp.h> > #include <linux/udp.h> > > diff --git a/include/net/icmp.h b/include/net/icmp.h > index 9ac2d2672a93..fd84adc47963 100644 > --- a/include/net/icmp.h > +++ b/include/net/icmp.h > @@ -46,7 +46,11 @@ static inline void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 > #if IS_ENABLED(CONFIG_NF_NAT) > void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info); > #else > -#define icmp_ndo_send icmp_send > +static inline void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) > +{ > + struct ip_options opts = { 0 }; > + __icmp_send(skb_in, type, code, info, &opts); > +} > #endif > > int icmp_rcv(struct sk_buff *skb); > diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c > index 396b492c804f..616e2dc1c8fa 100644 > --- a/net/ipv4/icmp.c > +++ b/net/ipv4/icmp.c > @@ -775,13 +775,14 @@ EXPORT_SYMBOL(__icmp_send); > void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) > { > struct sk_buff *cloned_skb = NULL; > + struct ip_options opts = { 0 }; > enum ip_conntrack_info ctinfo; > struct nf_conn *ct; > __be32 orig_ip; > > ct = nf_ct_get(skb_in, &ctinfo); > if (!ct || !(ct->status & IPS_SRC_NAT)) { > - icmp_send(skb_in, type, code, info); > + __icmp_send(skb_in, type, code, info, &opts); > return; > } > > @@ -796,7 +797,7 @@ void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) > > orig_ip = ip_hdr(skb_in)->saddr; > ip_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.ip; > - icmp_send(skb_in, type, code, info); > + __icmp_send(skb_in, type, code, info, &opts); > ip_hdr(skb_in)->saddr = orig_ip; > out: > consume_skb(cloned_skb); > diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c > index f3d05866692e..fd1f896115c1 100644 > --- a/net/ipv6/icmp.c > +++ b/net/ipv6/icmp.c > @@ -331,10 +331,9 @@ static int icmpv6_getfrag(void *from, char *to, int offset, int len, int odd, st > } > > #if IS_ENABLED(CONFIG_IPV6_MIP6) > -static void mip6_addr_swap(struct sk_buff *skb) > +static void mip6_addr_swap(struct sk_buff *skb, const struct inet6_skb_parm *opt) > { > struct ipv6hdr *iph = ipv6_hdr(skb); > - struct inet6_skb_parm *opt = IP6CB(skb); > struct ipv6_destopt_hao *hao; > struct in6_addr tmp; > int off; > @@ -351,7 +350,7 @@ static void mip6_addr_swap(struct sk_buff *skb) > } > } > #else > -static inline void mip6_addr_swap(struct sk_buff *skb) {} > +static inline void mip6_addr_swap(struct sk_buff *skb, const struct inet6_skb_parm *opt) {} > #endif > > static struct dst_entry *icmpv6_route_lookup(struct net *net, > @@ -446,7 +445,8 @@ static int icmp6_iif(const struct sk_buff *skb) > * Send an ICMP message in response to a packet in error > */ > void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > - const struct in6_addr *force_saddr) > + const struct in6_addr *force_saddr, > + const struct inet6_skb_parm *parm) > { > struct inet6_dev *idev = NULL; > struct ipv6hdr *hdr = ipv6_hdr(skb); > @@ -542,7 +542,7 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > if (!(skb->dev->flags & IFF_LOOPBACK) && !icmpv6_global_allow(net, type)) > goto out_bh_enable; > > - mip6_addr_swap(skb); > + mip6_addr_swap(skb, parm); > > sk = icmpv6_xmit_lock(net); > if (!sk) > @@ -559,7 +559,7 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > /* select a more meaningful saddr from input if */ > struct net_device *in_netdev; > > - in_netdev = dev_get_by_index(net, IP6CB(skb)->iif); > + in_netdev = dev_get_by_index(net, parm->iif); > if (in_netdev) { > ipv6_dev_get_saddr(net, in_netdev, &fl6.daddr, > inet6_sk(sk)->srcprefs, > @@ -640,7 +640,7 @@ EXPORT_SYMBOL(icmp6_send); > */ > void icmpv6_param_prob(struct sk_buff *skb, u8 code, int pos) > { > - icmp6_send(skb, ICMPV6_PARAMPROB, code, pos, NULL); > + icmp6_send(skb, ICMPV6_PARAMPROB, code, pos, NULL, IP6CB(skb)); > kfree_skb(skb); > } > > @@ -697,10 +697,10 @@ int ip6_err_gen_icmpv6_unreach(struct sk_buff *skb, int nhs, int type, > } > if (type == ICMP_TIME_EXCEEDED) > icmp6_send(skb2, ICMPV6_TIME_EXCEED, ICMPV6_EXC_HOPLIMIT, > - info, &temp_saddr); > + info, &temp_saddr, IP6CB(skb2)); > else > icmp6_send(skb2, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, > - info, &temp_saddr); > + info, &temp_saddr, IP6CB(skb2)); > if (rt) > ip6_rt_put(rt); > > diff --git a/net/ipv6/ip6_icmp.c b/net/ipv6/ip6_icmp.c > index 70c8c2f36c98..5f834ebc09b0 100644 > --- a/net/ipv6/ip6_icmp.c > +++ b/net/ipv6/ip6_icmp.c > @@ -40,7 +40,7 @@ void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) > rcu_read_lock(); > send = rcu_dereference(ip6_icmp_send); > if (send) > - send(skb, type, code, info, NULL); > + send(skb, type, code, info, NULL, IP6CB(skb)); > rcu_read_unlock(); > } > EXPORT_SYMBOL(icmpv6_send); > @@ -50,6 +50,7 @@ EXPORT_SYMBOL(icmpv6_send); > #include <net/netfilter/nf_conntrack.h> > void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > { > + struct inet6_skb_parm parm = { 0 }; > struct sk_buff *cloned_skb = NULL; > enum ip_conntrack_info ctinfo; > struct in6_addr orig_ip; > @@ -57,7 +58,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > > ct = nf_ct_get(skb_in, &ctinfo); > if (!ct || !(ct->status & IPS_SRC_NAT)) { > - icmpv6_send(skb_in, type, code, info); > + __icmpv6_send(skb_in, type, code, info, &parm); > return; > } > > @@ -72,7 +73,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > > orig_ip = ipv6_hdr(skb_in)->saddr; > ipv6_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.in6; > - icmpv6_send(skb_in, type, code, info); > + __icmpv6_send(skb_in, type, code, info, &parm); > ipv6_hdr(skb_in)->saddr = orig_ip; > out: > consume_skb(cloned_skb); > -- > 2.30.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 16:33 ` Willem de Bruijn @ 2021-02-18 17:35 ` Jason A. Donenfeld 2021-02-18 20:15 ` Willem de Bruijn 0 siblings, 1 reply; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-18 17:35 UTC (permalink / raw) To: Willem de Bruijn; +Cc: Network Development, SinYu On Thu, Feb 18, 2021 at 5:34 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > Thanks for respinning. > > Making ipv4 and ipv6 more aligned is a good goal, but more for > net-next than bug fixes that need to be backported to many stable > branches. > > Beyond that, I'm not sure this fixes additional cases vs the previous > patch? It uses new on-stack variables instead of skb->cb, which again > is probably good in general, but adds more change than is needed for > the stable fix. It doesn't appear to be problematic for applying to stable. I think this v2 is the "right way" to handle it. Zeroing out skb->cb is unexpected and weird anyway. What if the caller was expecting to use their skb->cb after calling icmp_ndo_send? Did they think it'd get wiped out like that? This v2 prevents that weird behavior from happening. > My comment on fixing all callers of icmp{,v6}_send was wrong, in > hindsight. In most cases IPCB is set correctly before calling those, > so we cannot just zero inside those. If we can only address the case > for icmp{,v6}_ndo_send I think the previous patch introduced less > churn, so is preferable. Unless I'm missing something. As mentioned above it's weird and unexpected. > Reminder of two main comments: sufficient to zero sizeof(IPCB..) and > if respinning, please explicitly mention the path that leads to a > stack overflow, as it is not immediately obvious (even from reading > the fix code?). I don't intend to respin v1, as I think v2 is more correct, and I don't think only zeroing IPCB is a smart idea, as in the future that code is bound to break when somebody forgets to update it. This v2 does away with the zeroing all together, though, so that the right bytes to be zeroed are properly enforced all the time by the type system. Jason ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 17:35 ` Jason A. Donenfeld @ 2021-02-18 20:15 ` Willem de Bruijn 2021-02-18 20:24 ` Jason A. Donenfeld 0 siblings, 1 reply; 16+ messages in thread From: Willem de Bruijn @ 2021-02-18 20:15 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: Network Development, SinYu On Thu, Feb 18, 2021 at 12:58 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Thu, Feb 18, 2021 at 5:34 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > Thanks for respinning. > > > > Making ipv4 and ipv6 more aligned is a good goal, but more for > > net-next than bug fixes that need to be backported to many stable > > branches. > > > > Beyond that, I'm not sure this fixes additional cases vs the previous > > patch? It uses new on-stack variables instead of skb->cb, which again > > is probably good in general, but adds more change than is needed for > > the stable fix. > > It doesn't appear to be problematic for applying to stable. I think > this v2 is the "right way" to handle it. Zeroing out skb->cb is > unexpected and weird anyway. What if the caller was expecting to use > their skb->cb after calling icmp_ndo_send? Did they think it'd get > wiped out like that? This v2 prevents that weird behavior from > happening. > > > My comment on fixing all callers of icmp{,v6}_send was wrong, in > > hindsight. In most cases IPCB is set correctly before calling those, > > so we cannot just zero inside those. If we can only address the case > > for icmp{,v6}_ndo_send I think the previous patch introduced less > > churn, so is preferable. Unless I'm missing something. > > As mentioned above it's weird and unexpected. > > > Reminder of two main comments: sufficient to zero sizeof(IPCB..) and > > if respinning, please explicitly mention the path that leads to a > > stack overflow, as it is not immediately obvious (even from reading > > the fix code?). > > I don't intend to respin v1, as I think v2 is more correct, and I > don't think only zeroing IPCB is a smart idea, as in the future that > code is bound to break when somebody forgets to update it. This v2 > does away with the zeroing all together, though, so that the right > bytes to be zeroed are properly enforced all the time by the type > system. I'm afraid this latest version seems to have build issues, as per the patchwork bot. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 20:15 ` Willem de Bruijn @ 2021-02-18 20:24 ` Jason A. Donenfeld 2021-02-18 20:34 ` [PATCH net v3] " Jason A. Donenfeld 2021-02-18 20:36 ` [PATCH net v2] " Willem de Bruijn 0 siblings, 2 replies; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-18 20:24 UTC (permalink / raw) To: Willem de Bruijn; +Cc: Network Development, SinYu On Thu, Feb 18, 2021 at 9:16 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Thu, Feb 18, 2021 at 12:58 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > On Thu, Feb 18, 2021 at 5:34 PM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > Thanks for respinning. > > > > > > Making ipv4 and ipv6 more aligned is a good goal, but more for > > > net-next than bug fixes that need to be backported to many stable > > > branches. > > > > > > Beyond that, I'm not sure this fixes additional cases vs the previous > > > patch? It uses new on-stack variables instead of skb->cb, which again > > > is probably good in general, but adds more change than is needed for > > > the stable fix. > > > > It doesn't appear to be problematic for applying to stable. I think > > this v2 is the "right way" to handle it. Zeroing out skb->cb is > > unexpected and weird anyway. What if the caller was expecting to use > > their skb->cb after calling icmp_ndo_send? Did they think it'd get > > wiped out like that? This v2 prevents that weird behavior from > > happening. > > > > > My comment on fixing all callers of icmp{,v6}_send was wrong, in > > > hindsight. In most cases IPCB is set correctly before calling those, > > > so we cannot just zero inside those. If we can only address the case > > > for icmp{,v6}_ndo_send I think the previous patch introduced less > > > churn, so is preferable. Unless I'm missing something. > > > > As mentioned above it's weird and unexpected. > > > > > Reminder of two main comments: sufficient to zero sizeof(IPCB..) and > > > if respinning, please explicitly mention the path that leads to a > > > stack overflow, as it is not immediately obvious (even from reading > > > the fix code?). > > > > I don't intend to respin v1, as I think v2 is more correct, and I > > don't think only zeroing IPCB is a smart idea, as in the future that > > code is bound to break when somebody forgets to update it. This v2 > > does away with the zeroing all together, though, so that the right > > bytes to be zeroed are properly enforced all the time by the type > > system. > > I'm afraid this latest version seems to have build issues, as per the > patchwork bot. Hmm I didn't get those bot emails. Either way, I'll do a bit of build testing with different config knobs now and send a v3. Thanks for letting me know. Jason ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net v3] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 20:24 ` Jason A. Donenfeld @ 2021-02-18 20:34 ` Jason A. Donenfeld 2021-02-18 22:06 ` Willem de Bruijn 2021-02-18 20:36 ` [PATCH net v2] " Willem de Bruijn 1 sibling, 1 reply; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-18 20:34 UTC (permalink / raw) To: netdev; +Cc: Jason A. Donenfeld, SinYu, Willem de Bruijn The icmp{,v6}_send functions make all sorts of use of skb->cb, casting it with IPCB or IP6CB, 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. As a result, 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. This commit fixes the issue by passing an empty IPCB or IP6CB along to the functions that actually do the work. For the icmp_send, this was already trivial, thanks to __icmp_send providing the plumbing function. For icmpv6_send, this required a tiny bit of refactoring to make it behave like the v4 case, after which it was straight forward. Fixes: a2b78e9b2cac ("sunvnet: generate ICMP PTMUD messages for smaller port MTUs") Reported-by: SinYu <liuxyon@gmail.com> Cc: Willem de Bruijn <willemb@google.com> Link: https://lore.kernel.org/netdev/CAF=yD-LOF116aHub6RMe8vB8ZpnrrnoTdqhobEx+bvoA8AsP0w@mail.gmail.com/T/ Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- Changes v2->v3: - Fix build errors with CONFIG_IPV6=m drivers/net/gtp.c | 1 - include/linux/icmpv6.h | 26 ++++++++++++++++++++------ include/linux/ipv6.h | 1 - include/net/icmp.h | 6 +++++- net/ipv4/icmp.c | 5 +++-- net/ipv6/icmp.c | 18 +++++++++--------- net/ipv6/ip6_icmp.c | 12 +++++++----- 7 files changed, 44 insertions(+), 25 deletions(-) diff --git a/drivers/net/gtp.c b/drivers/net/gtp.c index 4c04e271f184..fd3c2d86e48b 100644 --- a/drivers/net/gtp.c +++ b/drivers/net/gtp.c @@ -539,7 +539,6 @@ static int gtp_build_skb_ip4(struct sk_buff *skb, struct net_device *dev, if (!skb_is_gso(skb) && (iph->frag_off & htons(IP_DF)) && mtu < ntohs(iph->tot_len)) { netdev_dbg(dev, "packet too big, fragmentation needed\n"); - memset(IPCB(skb), 0, sizeof(*IPCB(skb))); icmp_ndo_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED, htonl(mtu)); goto err_rt; diff --git a/include/linux/icmpv6.h b/include/linux/icmpv6.h index 1b3371ae8193..0a383202dd5e 100644 --- a/include/linux/icmpv6.h +++ b/include/linux/icmpv6.h @@ -3,6 +3,7 @@ #define _LINUX_ICMPV6_H #include <linux/skbuff.h> +#include <linux/ipv6.h> #include <uapi/linux/icmpv6.h> static inline struct icmp6hdr *icmp6_hdr(const struct sk_buff *skb) @@ -15,13 +16,16 @@ static inline struct icmp6hdr *icmp6_hdr(const struct sk_buff *skb) #if IS_ENABLED(CONFIG_IPV6) typedef void ip6_icmp_send_t(struct sk_buff *skb, u8 type, u8 code, __u32 info, - const struct in6_addr *force_saddr); + const struct in6_addr *force_saddr, + const struct inet6_skb_parm *parm); #if IS_BUILTIN(CONFIG_IPV6) void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, - const struct in6_addr *force_saddr); -static inline void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) + const struct in6_addr *force_saddr, + const struct inet6_skb_parm *parm); +static inline void __icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, + const struct inet6_skb_parm *parm) { - icmp6_send(skb, type, code, info, NULL); + icmp6_send(skb, type, code, info, NULL, parm); } static inline int inet6_register_icmp_sender(ip6_icmp_send_t *fn) { @@ -34,18 +38,28 @@ static inline int inet6_unregister_icmp_sender(ip6_icmp_send_t *fn) return 0; } #else -extern void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info); +extern void __icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, + const struct inet6_skb_parm *parm); extern int inet6_register_icmp_sender(ip6_icmp_send_t *fn); extern int inet6_unregister_icmp_sender(ip6_icmp_send_t *fn); #endif +static inline void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) +{ + __icmpv6_send(skb, type, code, info, IP6CB(skb)); +} + int ip6_err_gen_icmpv6_unreach(struct sk_buff *skb, int nhs, int type, unsigned int data_len); #if IS_ENABLED(CONFIG_NF_NAT) void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info); #else -#define icmpv6_ndo_send icmpv6_send +static inline void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) +{ + struct inet6_skb_parm parm = { 0 }; + __icmpv6_send(skb_in, type, code, info, &parm); +} #endif #else diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h index dda61d150a13..f514a7dd8c9c 100644 --- a/include/linux/ipv6.h +++ b/include/linux/ipv6.h @@ -84,7 +84,6 @@ struct ipv6_params { __s32 autoconf; }; extern struct ipv6_params ipv6_defaults; -#include <linux/icmpv6.h> #include <linux/tcp.h> #include <linux/udp.h> diff --git a/include/net/icmp.h b/include/net/icmp.h index 9ac2d2672a93..fd84adc47963 100644 --- a/include/net/icmp.h +++ b/include/net/icmp.h @@ -46,7 +46,11 @@ static inline void icmp_send(struct sk_buff *skb_in, int type, int code, __be32 #if IS_ENABLED(CONFIG_NF_NAT) void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info); #else -#define icmp_ndo_send icmp_send +static inline void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) +{ + struct ip_options opts = { 0 }; + __icmp_send(skb_in, type, code, info, &opts); +} #endif int icmp_rcv(struct sk_buff *skb); diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c index 396b492c804f..616e2dc1c8fa 100644 --- a/net/ipv4/icmp.c +++ b/net/ipv4/icmp.c @@ -775,13 +775,14 @@ EXPORT_SYMBOL(__icmp_send); void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) { struct sk_buff *cloned_skb = NULL; + struct ip_options opts = { 0 }; enum ip_conntrack_info ctinfo; struct nf_conn *ct; __be32 orig_ip; ct = nf_ct_get(skb_in, &ctinfo); if (!ct || !(ct->status & IPS_SRC_NAT)) { - icmp_send(skb_in, type, code, info); + __icmp_send(skb_in, type, code, info, &opts); return; } @@ -796,7 +797,7 @@ void icmp_ndo_send(struct sk_buff *skb_in, int type, int code, __be32 info) orig_ip = ip_hdr(skb_in)->saddr; ip_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.ip; - icmp_send(skb_in, type, code, info); + __icmp_send(skb_in, type, code, info, &opts); ip_hdr(skb_in)->saddr = orig_ip; out: consume_skb(cloned_skb); diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c index f3d05866692e..fd1f896115c1 100644 --- a/net/ipv6/icmp.c +++ b/net/ipv6/icmp.c @@ -331,10 +331,9 @@ static int icmpv6_getfrag(void *from, char *to, int offset, int len, int odd, st } #if IS_ENABLED(CONFIG_IPV6_MIP6) -static void mip6_addr_swap(struct sk_buff *skb) +static void mip6_addr_swap(struct sk_buff *skb, const struct inet6_skb_parm *opt) { struct ipv6hdr *iph = ipv6_hdr(skb); - struct inet6_skb_parm *opt = IP6CB(skb); struct ipv6_destopt_hao *hao; struct in6_addr tmp; int off; @@ -351,7 +350,7 @@ static void mip6_addr_swap(struct sk_buff *skb) } } #else -static inline void mip6_addr_swap(struct sk_buff *skb) {} +static inline void mip6_addr_swap(struct sk_buff *skb, const struct inet6_skb_parm *opt) {} #endif static struct dst_entry *icmpv6_route_lookup(struct net *net, @@ -446,7 +445,8 @@ static int icmp6_iif(const struct sk_buff *skb) * Send an ICMP message in response to a packet in error */ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, - const struct in6_addr *force_saddr) + const struct in6_addr *force_saddr, + const struct inet6_skb_parm *parm) { struct inet6_dev *idev = NULL; struct ipv6hdr *hdr = ipv6_hdr(skb); @@ -542,7 +542,7 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, if (!(skb->dev->flags & IFF_LOOPBACK) && !icmpv6_global_allow(net, type)) goto out_bh_enable; - mip6_addr_swap(skb); + mip6_addr_swap(skb, parm); sk = icmpv6_xmit_lock(net); if (!sk) @@ -559,7 +559,7 @@ void icmp6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, /* select a more meaningful saddr from input if */ struct net_device *in_netdev; - in_netdev = dev_get_by_index(net, IP6CB(skb)->iif); + in_netdev = dev_get_by_index(net, parm->iif); if (in_netdev) { ipv6_dev_get_saddr(net, in_netdev, &fl6.daddr, inet6_sk(sk)->srcprefs, @@ -640,7 +640,7 @@ EXPORT_SYMBOL(icmp6_send); */ void icmpv6_param_prob(struct sk_buff *skb, u8 code, int pos) { - icmp6_send(skb, ICMPV6_PARAMPROB, code, pos, NULL); + icmp6_send(skb, ICMPV6_PARAMPROB, code, pos, NULL, IP6CB(skb)); kfree_skb(skb); } @@ -697,10 +697,10 @@ int ip6_err_gen_icmpv6_unreach(struct sk_buff *skb, int nhs, int type, } if (type == ICMP_TIME_EXCEEDED) icmp6_send(skb2, ICMPV6_TIME_EXCEED, ICMPV6_EXC_HOPLIMIT, - info, &temp_saddr); + info, &temp_saddr, IP6CB(skb2)); else icmp6_send(skb2, ICMPV6_DEST_UNREACH, ICMPV6_ADDR_UNREACH, - info, &temp_saddr); + info, &temp_saddr, IP6CB(skb2)); if (rt) ip6_rt_put(rt); diff --git a/net/ipv6/ip6_icmp.c b/net/ipv6/ip6_icmp.c index 70c8c2f36c98..995b2537c05f 100644 --- a/net/ipv6/ip6_icmp.c +++ b/net/ipv6/ip6_icmp.c @@ -33,23 +33,25 @@ int inet6_unregister_icmp_sender(ip6_icmp_send_t *fn) } EXPORT_SYMBOL(inet6_unregister_icmp_sender); -void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) +void __icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, + const struct inet6_skb_parm *parm) { ip6_icmp_send_t *send; rcu_read_lock(); send = rcu_dereference(ip6_icmp_send); if (send) - send(skb, type, code, info, NULL); + send(skb, type, code, info, NULL, IP6CB(skb)); rcu_read_unlock(); } -EXPORT_SYMBOL(icmpv6_send); +EXPORT_SYMBOL(__icmpv6_send); #endif #if IS_ENABLED(CONFIG_NF_NAT) #include <net/netfilter/nf_conntrack.h> void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) { + struct inet6_skb_parm parm = { 0 }; struct sk_buff *cloned_skb = NULL; enum ip_conntrack_info ctinfo; struct in6_addr orig_ip; @@ -57,7 +59,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) ct = nf_ct_get(skb_in, &ctinfo); if (!ct || !(ct->status & IPS_SRC_NAT)) { - icmpv6_send(skb_in, type, code, info); + __icmpv6_send(skb_in, type, code, info, &parm); return; } @@ -72,7 +74,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) orig_ip = ipv6_hdr(skb_in)->saddr; ipv6_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.in6; - icmpv6_send(skb_in, type, code, info); + __icmpv6_send(skb_in, type, code, info, &parm); ipv6_hdr(skb_in)->saddr = orig_ip; out: consume_skb(cloned_skb); -- 2.30.1 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net v3] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 20:34 ` [PATCH net v3] " Jason A. Donenfeld @ 2021-02-18 22:06 ` Willem de Bruijn 2021-02-19 2:03 ` Jason A. Donenfeld 0 siblings, 1 reply; 16+ messages in thread From: Willem de Bruijn @ 2021-02-18 22:06 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: Network Development, SinYu, Willem de Bruijn On Thu, Feb 18, 2021 at 3:39 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > The icmp{,v6}_send functions make all sorts of use of skb->cb, casting Again, if respinning, please briefly describe the specific buggy code path. I think it's informative and cannot be gleaned from the fix. > it with IPCB or IP6CB, 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. As a result, 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. > > This commit fixes the issue by passing an empty IPCB or IP6CB along to > the functions that actually do the work. For the icmp_send, this was > already trivial, thanks to __icmp_send providing the plumbing function. > For icmpv6_send, this required a tiny bit of refactoring to make it > behave like the v4 case, after which it was straight forward. > > Fixes: a2b78e9b2cac ("sunvnet: generate ICMP PTMUD messages for smaller port MTUs") > Reported-by: SinYu <liuxyon@gmail.com> > Cc: Willem de Bruijn <willemb@google.com> > Link: https://lore.kernel.org/netdev/CAF=yD-LOF116aHub6RMe8vB8ZpnrrnoTdqhobEx+bvoA8AsP0w@mail.gmail.com/T/ > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > -#define icmpv6_ndo_send icmpv6_send > +static inline void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > +{ > + struct inet6_skb_parm parm = { 0 }; > + __icmpv6_send(skb_in, type, code, info, &parm); > +} > #endif > -void icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info) > +void __icmpv6_send(struct sk_buff *skb, u8 type, u8 code, __u32 info, > + const struct inet6_skb_parm *parm) > { > ip6_icmp_send_t *send; > > rcu_read_lock(); > send = rcu_dereference(ip6_icmp_send); > if (send) > - send(skb, type, code, info, NULL); > + send(skb, type, code, info, NULL, IP6CB(skb)); This should be parm. > #if IS_ENABLED(CONFIG_NF_NAT) > #include <net/netfilter/nf_conntrack.h> > void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > { > + struct inet6_skb_parm parm = { 0 }; > struct sk_buff *cloned_skb = NULL; > enum ip_conntrack_info ctinfo; > struct in6_addr orig_ip; > @@ -57,7 +59,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > > ct = nf_ct_get(skb_in, &ctinfo); > if (!ct || !(ct->status & IPS_SRC_NAT)) { > - icmpv6_send(skb_in, type, code, info); > + __icmpv6_send(skb_in, type, code, info, &parm); > return; > } > > @@ -72,7 +74,7 @@ void icmpv6_ndo_send(struct sk_buff *skb_in, u8 type, u8 code, __u32 info) > > orig_ip = ipv6_hdr(skb_in)->saddr; > ipv6_hdr(skb_in)->saddr = ct->tuplehash[0].tuple.src.u3.in6; > - icmpv6_send(skb_in, type, code, info); > + __icmpv6_send(skb_in, type, code, info, &parm); > ipv6_hdr(skb_in)->saddr = orig_ip; > out: > consume_skb(cloned_skb); > -- > 2.30.1 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v3] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 22:06 ` Willem de Bruijn @ 2021-02-19 2:03 ` Jason A. Donenfeld 0 siblings, 0 replies; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-19 2:03 UTC (permalink / raw) To: Willem de Bruijn; +Cc: Network Development, SinYu, Willem de Bruijn On Thu, Feb 18, 2021 at 11:06 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Thu, Feb 18, 2021 at 3:39 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > The icmp{,v6}_send functions make all sorts of use of skb->cb, casting > > Again, if respinning, please briefly describe the specific buggy code > path. I think it's informative and cannot be gleaned from the fix. Ack. > > - send(skb, type, code, info, NULL); > > + send(skb, type, code, info, NULL, IP6CB(skb)); > > This should be parm. Nice catch, thanks. v4 coming up. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 20:24 ` Jason A. Donenfeld 2021-02-18 20:34 ` [PATCH net v3] " Jason A. Donenfeld @ 2021-02-18 20:36 ` Willem de Bruijn 2021-02-18 20:39 ` Jason A. Donenfeld 1 sibling, 1 reply; 16+ messages in thread From: Willem de Bruijn @ 2021-02-18 20:36 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: Network Development, SinYu On Thu, Feb 18, 2021 at 3:25 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > On Thu, Feb 18, 2021 at 9:16 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Thu, Feb 18, 2021 at 12:58 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > On Thu, Feb 18, 2021 at 5:34 PM Willem de Bruijn > > > <willemdebruijn.kernel@gmail.com> wrote: > > > > Thanks for respinning. > > > > > > > > Making ipv4 and ipv6 more aligned is a good goal, but more for > > > > net-next than bug fixes that need to be backported to many stable > > > > branches. > > > > > > > > Beyond that, I'm not sure this fixes additional cases vs the previous > > > > patch? It uses new on-stack variables instead of skb->cb, which again > > > > is probably good in general, but adds more change than is needed for > > > > the stable fix. > > > > > > It doesn't appear to be problematic for applying to stable. I think > > > this v2 is the "right way" to handle it. Zeroing out skb->cb is > > > unexpected and weird anyway. What if the caller was expecting to use > > > their skb->cb after calling icmp_ndo_send? Did they think it'd get > > > wiped out like that? This v2 prevents that weird behavior from > > > happening. > > > > > > > My comment on fixing all callers of icmp{,v6}_send was wrong, in > > > > hindsight. In most cases IPCB is set correctly before calling those, > > > > so we cannot just zero inside those. If we can only address the case > > > > for icmp{,v6}_ndo_send I think the previous patch introduced less > > > > churn, so is preferable. Unless I'm missing something. > > > > > > As mentioned above it's weird and unexpected. > > > > > > > Reminder of two main comments: sufficient to zero sizeof(IPCB..) and > > > > if respinning, please explicitly mention the path that leads to a > > > > stack overflow, as it is not immediately obvious (even from reading > > > > the fix code?). > > > > > > I don't intend to respin v1, as I think v2 is more correct, and I > > > don't think only zeroing IPCB is a smart idea, as in the future that > > > code is bound to break when somebody forgets to update it. This v2 > > > does away with the zeroing all together, though, so that the right > > > bytes to be zeroed are properly enforced all the time by the type > > > system. > > > > I'm afraid this latest version seems to have build issues, as per the > > patchwork bot. > > Hmm I didn't get those bot emails. Either way, I'll do a bit of build > testing with different config knobs now and send a v3. Thanks for > letting me know. Different bot :) You might get emails from the other later. These can be found through patchwork at https://patchwork.kernel.org/project/netdevbpf/patch/20210218160745.2343501-1-Jason@zx2c4.com/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v2] net: icmp: pass zeroed opts from icmp{,v6}_ndo_send before sending 2021-02-18 20:36 ` [PATCH net v2] " Willem de Bruijn @ 2021-02-18 20:39 ` Jason A. Donenfeld 0 siblings, 0 replies; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-18 20:39 UTC (permalink / raw) To: Willem de Bruijn; +Cc: Network Development, SinYu On Thu, Feb 18, 2021 at 9:37 PM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Thu, Feb 18, 2021 at 3:25 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > On Thu, Feb 18, 2021 at 9:16 PM Willem de Bruijn > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > > On Thu, Feb 18, 2021 at 12:58 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > > > > > > > On Thu, Feb 18, 2021 at 5:34 PM Willem de Bruijn > > > > <willemdebruijn.kernel@gmail.com> wrote: > > > > > Thanks for respinning. > > > > > > > > > > Making ipv4 and ipv6 more aligned is a good goal, but more for > > > > > net-next than bug fixes that need to be backported to many stable > > > > > branches. > > > > > > > > > > Beyond that, I'm not sure this fixes additional cases vs the previous > > > > > patch? It uses new on-stack variables instead of skb->cb, which again > > > > > is probably good in general, but adds more change than is needed for > > > > > the stable fix. > > > > > > > > It doesn't appear to be problematic for applying to stable. I think > > > > this v2 is the "right way" to handle it. Zeroing out skb->cb is > > > > unexpected and weird anyway. What if the caller was expecting to use > > > > their skb->cb after calling icmp_ndo_send? Did they think it'd get > > > > wiped out like that? This v2 prevents that weird behavior from > > > > happening. > > > > > > > > > My comment on fixing all callers of icmp{,v6}_send was wrong, in > > > > > hindsight. In most cases IPCB is set correctly before calling those, > > > > > so we cannot just zero inside those. If we can only address the case > > > > > for icmp{,v6}_ndo_send I think the previous patch introduced less > > > > > churn, so is preferable. Unless I'm missing something. > > > > > > > > As mentioned above it's weird and unexpected. > > > > > > > > > Reminder of two main comments: sufficient to zero sizeof(IPCB..) and > > > > > if respinning, please explicitly mention the path that leads to a > > > > > stack overflow, as it is not immediately obvious (even from reading > > > > > the fix code?). > > > > > > > > I don't intend to respin v1, as I think v2 is more correct, and I > > > > don't think only zeroing IPCB is a smart idea, as in the future that > > > > code is bound to break when somebody forgets to update it. This v2 > > > > does away with the zeroing all together, though, so that the right > > > > bytes to be zeroed are properly enforced all the time by the type > > > > system. > > > > > > I'm afraid this latest version seems to have build issues, as per the > > > patchwork bot. > > > > Hmm I didn't get those bot emails. Either way, I'll do a bit of build > > testing with different config knobs now and send a v3. Thanks for > > letting me know. > > Different bot :) > > You might get emails from the other later. These can be found through > patchwork at > > https://patchwork.kernel.org/project/netdevbpf/patch/20210218160745.2343501-1-Jason@zx2c4.com/ Wow, that is an awesome bot! Can't believe I hadn't seen that. v3 is posted at https://patchwork.kernel.org/project/netdevbpf/patch/20210218203404.2429186-1-Jason@zx2c4.com/ and I guess now we're waiting for the bots to get whirling on it. Jason ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending 2021-02-18 15:40 ` Jason A. Donenfeld 2021-02-18 16:07 ` [PATCH net v2] net: icmp: pass zeroed opts from " Jason A. Donenfeld @ 2021-02-18 17:29 ` Willem de Bruijn 1 sibling, 0 replies; 16+ messages in thread From: Willem de Bruijn @ 2021-02-18 17:29 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: Network Development, SinYu On Thu, Feb 18, 2021 at 10:40 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Willem, > > On Thu, Feb 18, 2021 at 3:57 PM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Thu, Feb 18, 2021 at 7:31 AM Jason A. Donenfeld <Jason@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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending 2021-02-18 12:30 [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending Jason A. Donenfeld 2021-02-18 14:56 ` Willem de Bruijn @ 2021-02-18 19:08 ` Jakub Kicinski 2021-02-18 19:24 ` Jason A. Donenfeld 1 sibling, 1 reply; 16+ messages in thread From: Jakub Kicinski @ 2021-02-18 19:08 UTC (permalink / raw) To: Jason A. Donenfeld; +Cc: netdev, SinYu, Willem de Bruijn On Thu, 18 Feb 2021 13:30:53 +0100 Jason A. Donenfeld wrote: > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending 2021-02-18 19:08 ` Jakub Kicinski @ 2021-02-18 19:24 ` Jason A. Donenfeld 0 siblings, 0 replies; 16+ messages in thread From: Jason A. Donenfeld @ 2021-02-18 19:24 UTC (permalink / raw) To: Jakub Kicinski; +Cc: Netdev, SinYu, Willem de Bruijn On Thu, Feb 18, 2021 at 8:08 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Thu, 18 Feb 2021 13:30:53 +0100 Jason A. Donenfeld wrote: > > 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-02-19 2:04 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-18 12:30 [PATCH net] net: icmp: zero-out cb in icmp{,v6}_ndo_send before sending Jason A. Donenfeld 2021-02-18 14:56 ` Willem de Bruijn 2021-02-18 15:40 ` Jason A. Donenfeld 2021-02-18 16:07 ` [PATCH net v2] net: icmp: pass zeroed opts from " Jason A. Donenfeld 2021-02-18 16:33 ` Willem de Bruijn 2021-02-18 17:35 ` Jason A. Donenfeld 2021-02-18 20:15 ` Willem de Bruijn 2021-02-18 20:24 ` Jason A. Donenfeld 2021-02-18 20:34 ` [PATCH net v3] " Jason A. Donenfeld 2021-02-18 22:06 ` Willem de Bruijn 2021-02-19 2:03 ` Jason A. Donenfeld 2021-02-18 20:36 ` [PATCH net v2] " Willem de Bruijn 2021-02-18 20:39 ` Jason A. Donenfeld 2021-02-18 17:29 ` [PATCH net] net: icmp: zero-out cb in " Willem de Bruijn 2021-02-18 19:08 ` Jakub Kicinski 2021-02-18 19:24 ` Jason A. Donenfeld
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.