| Message ID | 20230801064318.34408-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 30e0191b16e8a58e4620fa3e2839ddc7b9d4281c |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] ip6mr: Fix skb_under_panic in ip6mr_cache_report() | expand |
On Tue, Aug 1, 2023 at 8:45 AM Yue Haibing <yuehaibing@huawei.com> wrote: > > skbuff: skb_under_panic: text:ffffffff88771f69 len:56 put:-4 > head:ffff88805f86a800 data:ffff887f5f86a850 tail:0x88 end:0x2c0 dev:pim6reg > > When setup a vlan device on dev pim6reg, DAD ns packet may sent on reg_vif_xmit(). > reg_vif_xmit() > ip6mr_cache_report() > skb_push(skb, -skb_network_offset(pkt));//skb_network_offset(pkt) is 4 > And skb_push declared as: > void *skb_push(struct sk_buff *skb, unsigned int len); > skb->data -= len; > //0xffff88805f86a84c - 0xfffffffc = 0xffff887f5f86a850 > skb->data is set to 0xffff887f5f86a850, which is invalid mem addr, lead to skb_push() fails. > > Fixes: 14fb64e1f449 ("[IPV6] MROUTE: Support PIM-SM (SSM).") > Signed-off-by: Yue Haibing <yuehaibing@huawei.com> > --- > v3: drop unnecessary nhoff change > v2: Use __skb_pull() and fix commit log. > --- > net/ipv6/ip6mr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c > index cc3d5ad17257..67a3b8f6e72b 100644 > --- a/net/ipv6/ip6mr.c > +++ b/net/ipv6/ip6mr.c > @@ -1073,7 +1073,7 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt, > And all this only to mangle msg->im6_msgtype and > to set msg->im6_mbz to "mbz" :-) > */ > - skb_push(skb, -skb_network_offset(pkt)); > + __skb_pull(skb, skb_network_offset(pkt)); > > skb_push(skb, sizeof(*msg)); > skb_reset_transport_header(skb); Presumably this code has never been tested :/ Reviewed-by: Eric Dumazet <edumazet@google.com>
On Tue, 1 Aug 2023 09:51:29 +0200 Eric Dumazet wrote: > > - skb_push(skb, -skb_network_offset(pkt)); > > + __skb_pull(skb, skb_network_offset(pkt)); > > > > skb_push(skb, sizeof(*msg)); > > skb_reset_transport_header(skb); > > Presumably this code has never been tested :/ Could have been tested on 32bit, I wonder if there is more such bugs :S
On 8/1/23 2:11 PM, Jakub Kicinski wrote: > On Tue, 1 Aug 2023 09:51:29 +0200 Eric Dumazet wrote: >>> - skb_push(skb, -skb_network_offset(pkt)); >>> + __skb_pull(skb, skb_network_offset(pkt)); >>> >>> skb_push(skb, sizeof(*msg)); >>> skb_reset_transport_header(skb); >> >> Presumably this code has never been tested :/ > > Could have been tested on 32bit, I wonder if there is more such bugs :S that pattern shows up a few times: net/ipv4/ah4.c: skb_push(skb, -skb_network_offset(skb)); net/ipv4/esp4.c: skb_push(skb, -skb_network_offset(skb)); net/ipv4/esp4_offload.c: skb_push(skb, -skb_network_offset(skb)); net/ipv4/esp4_offload.c: skb_push(skb, -skb_network_offset(skb)); net/ipv4/xfrm4_tunnel.c: skb_push(skb, -skb_network_offset(skb)); net/ipv6/ah6.c: skb_push(skb, -skb_network_offset(skb)); net/ipv6/esp6.c: skb_push(skb, -skb_network_offset(skb)); net/ipv6/esp6_offload.c: skb_push(skb, -skb_network_offset(skb)); net/ipv6/esp6_offload.c: skb_push(skb, -skb_network_offset(skb)); net/ipv6/ip6mr.c: skb_push(skb, -skb_network_offset(pkt)); net/ipv6/mip6.c: skb_push(skb, -skb_network_offset(skb)); net/ipv6/mip6.c: skb_push(skb, -skb_network_offset(skb)); net/ipv6/xfrm6_tunnel.c: skb_push(skb, -skb_network_offset(skb)); net/xfrm/xfrm_ipcomp.c: skb_push(skb, -skb_network_offset(skb));
On 2023/8/2 8:52, David Ahern wrote: > On 8/1/23 2:11 PM, Jakub Kicinski wrote: >> On Tue, 1 Aug 2023 09:51:29 +0200 Eric Dumazet wrote: >>>> - skb_push(skb, -skb_network_offset(pkt)); >>>> + __skb_pull(skb, skb_network_offset(pkt)); >>>> >>>> skb_push(skb, sizeof(*msg)); >>>> skb_reset_transport_header(skb); >>> >>> Presumably this code has never been tested :/ >> >> Could have been tested on 32bit, I wonder if there is more such bugs :S > > that pattern shows up a few times: Ok, I will test and fix these if any. > > net/ipv4/ah4.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv4/esp4.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv4/esp4_offload.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv4/esp4_offload.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv4/xfrm4_tunnel.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv6/ah6.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv6/esp6.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv6/esp6_offload.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv6/esp6_offload.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv6/ip6mr.c: skb_push(skb, -skb_network_offset(pkt)); > net/ipv6/mip6.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv6/mip6.c: skb_push(skb, -skb_network_offset(skb)); > net/ipv6/xfrm6_tunnel.c: skb_push(skb, -skb_network_offset(skb)); > net/xfrm/xfrm_ipcomp.c: skb_push(skb, -skb_network_offset(skb)); > . >
On Wed, 2 Aug 2023 09:28:31 +0800 YueHaibing wrote: > > that pattern shows up a few times: > > Ok, I will test and fix these if any. Thanks, we may also want to add a DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in skb_push() but perhaps that's an overkill. Most of those cases are probably legit (skb_.*_offset() can well be negative if headers were already pulled).
On 8/1/23 8:10 PM, Jakub Kicinski wrote: > On Wed, 2 Aug 2023 09:28:31 +0800 YueHaibing wrote: >>> that pattern shows up a few times: >> >> Ok, I will test and fix these if any. > > Thanks, we may also want to add a > DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) > in skb_push() but perhaps that's an overkill. > Most of those cases are probably legit (skb_.*_offset() > can well be negative if headers were already pulled). Yea, a lot of those could be legit, so it would be best to have a test case that shows it can be triggered and then any patch fixes it.
Hello: This patch was applied to netdev/net.git (main) by David S. Miller <davem@davemloft.net>: On Tue, 1 Aug 2023 14:43:18 +0800 you wrote: > skbuff: skb_under_panic: text:ffffffff88771f69 len:56 put:-4 > head:ffff88805f86a800 data:ffff887f5f86a850 tail:0x88 end:0x2c0 dev:pim6reg > ------------[ cut here ]------------ > kernel BUG at net/core/skbuff.c:192! > invalid opcode: 0000 [#1] PREEMPT SMP KASAN > CPU: 2 PID: 22968 Comm: kworker/2:11 Not tainted 6.5.0-rc3-00044-g0a8db05b571a #236 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014 > Workqueue: ipv6_addrconf addrconf_dad_work > RIP: 0010:skb_panic+0x152/0x1d0 > Call Trace: > <TASK> > skb_push+0xc4/0xe0 > ip6mr_cache_report+0xd69/0x19b0 > reg_vif_xmit+0x406/0x690 > dev_hard_start_xmit+0x17e/0x6e0 > __dev_queue_xmit+0x2d6a/0x3d20 > vlan_dev_hard_start_xmit+0x3ab/0x5c0 > dev_hard_start_xmit+0x17e/0x6e0 > __dev_queue_xmit+0x2d6a/0x3d20 > neigh_connected_output+0x3ed/0x570 > ip6_finish_output2+0x5b5/0x1950 > ip6_finish_output+0x693/0x11c0 > ip6_output+0x24b/0x880 > NF_HOOK.constprop.0+0xfd/0x530 > ndisc_send_skb+0x9db/0x1400 > ndisc_send_rs+0x12a/0x6c0 > addrconf_dad_completed+0x3c9/0xea0 > addrconf_dad_work+0x849/0x1420 > process_one_work+0xa22/0x16e0 > worker_thread+0x679/0x10c0 > ret_from_fork+0x28/0x60 > ret_from_fork_asm+0x11/0x20 > > [...] Here is the summary with links: - [v3] ip6mr: Fix skb_under_panic in ip6mr_cache_report() https://git.kernel.org/netdev/net/c/30e0191b16e8 You are awesome, thank you!
diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c index cc3d5ad17257..67a3b8f6e72b 100644 --- a/net/ipv6/ip6mr.c +++ b/net/ipv6/ip6mr.c @@ -1073,7 +1073,7 @@ static int ip6mr_cache_report(const struct mr_table *mrt, struct sk_buff *pkt, And all this only to mangle msg->im6_msgtype and to set msg->im6_mbz to "mbz" :-) */ - skb_push(skb, -skb_network_offset(pkt)); + __skb_pull(skb, skb_network_offset(pkt)); skb_push(skb, sizeof(*msg)); skb_reset_transport_header(skb);
skbuff: skb_under_panic: text:ffffffff88771f69 len:56 put:-4 head:ffff88805f86a800 data:ffff887f5f86a850 tail:0x88 end:0x2c0 dev:pim6reg ------------[ cut here ]------------ kernel BUG at net/core/skbuff.c:192! invalid opcode: 0000 [#1] PREEMPT SMP KASAN CPU: 2 PID: 22968 Comm: kworker/2:11 Not tainted 6.5.0-rc3-00044-g0a8db05b571a #236 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014 Workqueue: ipv6_addrconf addrconf_dad_work RIP: 0010:skb_panic+0x152/0x1d0 Call Trace: <TASK> skb_push+0xc4/0xe0 ip6mr_cache_report+0xd69/0x19b0 reg_vif_xmit+0x406/0x690 dev_hard_start_xmit+0x17e/0x6e0 __dev_queue_xmit+0x2d6a/0x3d20 vlan_dev_hard_start_xmit+0x3ab/0x5c0 dev_hard_start_xmit+0x17e/0x6e0 __dev_queue_xmit+0x2d6a/0x3d20 neigh_connected_output+0x3ed/0x570 ip6_finish_output2+0x5b5/0x1950 ip6_finish_output+0x693/0x11c0 ip6_output+0x24b/0x880 NF_HOOK.constprop.0+0xfd/0x530 ndisc_send_skb+0x9db/0x1400 ndisc_send_rs+0x12a/0x6c0 addrconf_dad_completed+0x3c9/0xea0 addrconf_dad_work+0x849/0x1420 process_one_work+0xa22/0x16e0 worker_thread+0x679/0x10c0 ret_from_fork+0x28/0x60 ret_from_fork_asm+0x11/0x20 When setup a vlan device on dev pim6reg, DAD ns packet may sent on reg_vif_xmit(). reg_vif_xmit() ip6mr_cache_report() skb_push(skb, -skb_network_offset(pkt));//skb_network_offset(pkt) is 4 And skb_push declared as: void *skb_push(struct sk_buff *skb, unsigned int len); skb->data -= len; //0xffff88805f86a84c - 0xfffffffc = 0xffff887f5f86a850 skb->data is set to 0xffff887f5f86a850, which is invalid mem addr, lead to skb_push() fails. Fixes: 14fb64e1f449 ("[IPV6] MROUTE: Support PIM-SM (SSM).") Signed-off-by: Yue Haibing <yuehaibing@huawei.com> --- v3: drop unnecessary nhoff change v2: Use __skb_pull() and fix commit log. --- net/ipv6/ip6mr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)