* [PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets @ 2021-04-19 15:22 Davide Caratti 2021-04-19 15:23 ` [PATCH net 1/2] openvswitch: " Davide Caratti ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Davide Caratti @ 2021-04-19 15:22 UTC (permalink / raw) To: netdev, David S. Miller, Jakub Kicinski - patch 1/2 fixes openvswitch IPv4 fragmentation, that does a stack OOB read after commit d52e5a7e7ca4 ("ipv4: lock mtu in fnhe when received PMTU < net.ipv4.route.min_pmt") - patch 2/2 fixes the same issue in TC 'sch_frag' code Davide Caratti (2): openvswitch: fix stack OOB read while fragmenting IPv4 packets net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets net/openvswitch/actions.c | 8 ++++---- net/sched/sch_frag.c | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) -- 2.30.2 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting IPv4 packets 2021-04-19 15:22 [PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets Davide Caratti @ 2021-04-19 15:23 ` Davide Caratti 2021-04-21 9:27 ` Eelco Chaudron 2021-04-19 15:23 ` [PATCH net 2/2] net/sched: sch_frag: " Davide Caratti 2021-04-23 12:11 ` [PATCH net 0/2] " Davide Caratti 2 siblings, 1 reply; 12+ messages in thread From: Davide Caratti @ 2021-04-19 15:23 UTC (permalink / raw) To: Pravin B Shelar, David S. Miller, Jakub Kicinski, Sabrina Dubroca, netdev Cc: echaudro running openvswitch on kernels built with KASAN, it's possible to see the following splat while testing fragmentation of IPv4 packets: BUG: KASAN: stack-out-of-bounds in ip_do_fragment+0x1b03/0x1f60 Read of size 1 at addr ffff888112fc713c by task handler2/1367 CPU: 0 PID: 1367 Comm: handler2 Not tainted 5.12.0-rc6+ #418 Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014 Call Trace: dump_stack+0x92/0xc1 print_address_description.constprop.7+0x1a/0x150 kasan_report.cold.13+0x7f/0x111 ip_do_fragment+0x1b03/0x1f60 ovs_fragment+0x5bf/0x840 [openvswitch] do_execute_actions+0x1bd5/0x2400 [openvswitch] ovs_execute_actions+0xc8/0x3d0 [openvswitch] ovs_packet_cmd_execute+0xa39/0x1150 [openvswitch] genl_family_rcv_msg_doit.isra.15+0x227/0x2d0 genl_rcv_msg+0x287/0x490 netlink_rcv_skb+0x120/0x380 genl_rcv+0x24/0x40 netlink_unicast+0x439/0x630 netlink_sendmsg+0x719/0xbf0 sock_sendmsg+0xe2/0x110 ____sys_sendmsg+0x5ba/0x890 ___sys_sendmsg+0xe9/0x160 __sys_sendmsg+0xd3/0x170 do_syscall_64+0x33/0x40 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x7f957079db07 Code: c3 66 90 41 54 41 89 d4 55 48 89 f5 53 89 fb 48 83 ec 10 e8 eb ec ff ff 44 89 e2 48 89 ee 89 df 41 89 c0 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 35 44 89 c7 48 89 44 24 08 e8 24 ed ff ff 48 RSP: 002b:00007f956ce35a50 EFLAGS: 00000293 ORIG_RAX: 000000000000002e RAX: ffffffffffffffda RBX: 0000000000000019 RCX: 00007f957079db07 RDX: 0000000000000000 RSI: 00007f956ce35ae0 RDI: 0000000000000019 RBP: 00007f956ce35ae0 R08: 0000000000000000 R09: 00007f9558006730 R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000000 R13: 00007f956ce37308 R14: 00007f956ce35f80 R15: 00007f956ce35ae0 The buggy address belongs to the page: page:00000000af2a1d93 refcount:0 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x112fc7 flags: 0x17ffffc0000000() raw: 0017ffffc0000000 0000000000000000 dead000000000122 0000000000000000 raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000 page dumped because: kasan: bad access detected addr ffff888112fc713c is located in stack of task handler2/1367 at offset 180 in frame: ovs_fragment+0x0/0x840 [openvswitch] this frame has 2 objects: [32, 144) 'ovs_dst' [192, 424) 'ovs_rt' Memory state around the buggy address: ffff888112fc7000: f3 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffff888112fc7080: 00 f1 f1 f1 f1 00 00 00 00 00 00 00 00 00 00 00 >ffff888112fc7100: 00 00 00 f2 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 ^ ffff888112fc7180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffff888112fc7200: 00 00 00 00 00 00 f2 f2 f2 00 00 00 00 00 00 00 for IPv4 packets, ovs_fragment() uses a temporary struct dst_entry. Then, in the following call graph: ip_do_fragment() ip_skb_dst_mtu() ip_dst_mtu_maybe_forward() ip_mtu_locked() the pointer to struct dst_entry is used as pointer to struct rtable: this turns the access to struct members like rt_mtu_locked into an OOB read in the stack. Fix this changing the temporary variable used for IPv4 packets in ovs_fragment(), similarly to what is done for IPv6 few lines below. Fixes: d52e5a7e7ca4 ("ipv4: lock mtu in fnhe when received PMTU < net.ipv4.route.min_pmt") Cc: <stable@vger.kernel.org> Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/openvswitch/actions.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c index 92a0b67b2728..77d924ab8cdb 100644 --- a/net/openvswitch/actions.c +++ b/net/openvswitch/actions.c @@ -827,17 +827,17 @@ static void ovs_fragment(struct net *net, struct vport *vport, } if (key->eth.type == htons(ETH_P_IP)) { - struct dst_entry ovs_dst; + struct rtable ovs_rt = { 0 }; unsigned long orig_dst; prepare_frag(vport, skb, orig_network_offset, ovs_key_mac_proto(key)); - dst_init(&ovs_dst, &ovs_dst_ops, NULL, 1, + dst_init(&ovs_rt.dst, &ovs_dst_ops, NULL, 1, DST_OBSOLETE_NONE, DST_NOCOUNT); - ovs_dst.dev = vport->dev; + ovs_rt.dst.dev = vport->dev; orig_dst = skb->_skb_refdst; - skb_dst_set_noref(skb, &ovs_dst); + skb_dst_set_noref(skb, &ovs_rt.dst); IPCB(skb)->frag_max_size = mru; ip_do_fragment(net, skb->sk, skb, ovs_vport_output); -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting IPv4 packets 2021-04-19 15:23 ` [PATCH net 1/2] openvswitch: " Davide Caratti @ 2021-04-21 9:27 ` Eelco Chaudron 2021-04-21 15:05 ` Davide Caratti 0 siblings, 1 reply; 12+ messages in thread From: Eelco Chaudron @ 2021-04-21 9:27 UTC (permalink / raw) To: Davide Caratti Cc: Pravin B Shelar, David S. Miller, Jakub Kicinski, Sabrina Dubroca, netdev On 19 Apr 2021, at 17:23, Davide Caratti wrote: > running openvswitch on kernels built with KASAN, it's possible to see > the > following splat while testing fragmentation of IPv4 packets: <SNIP> > for IPv4 packets, ovs_fragment() uses a temporary struct dst_entry. > Then, > in the following call graph: > > ip_do_fragment() > ip_skb_dst_mtu() > ip_dst_mtu_maybe_forward() > ip_mtu_locked() > > the pointer to struct dst_entry is used as pointer to struct rtable: > this > turns the access to struct members like rt_mtu_locked into an OOB read > in > the stack. Fix this changing the temporary variable used for IPv4 > packets > in ovs_fragment(), similarly to what is done for IPv6 few lines below. > > Fixes: d52e5a7e7ca4 ("ipv4: lock mtu in fnhe when received PMTU < > net.ipv4.route.min_pmt") > Cc: <stable@vger.kernel.org> > Signed-off-by: Davide Caratti <dcaratti@redhat.com> The fix looks good to me, however isn’t the real root cause ip_mtu_locked() who casts struct dst_entry to struct rtable (not even using container_of())? I do not know details in this area of the code, so maybe it’s just fine to always assume dst_entry is part of a rtable struct, as I see other core functions do the same ipv4_neigh_lookup()/ipv4_confirm_neigh(). Acked-by: Eelco Chaudron <echaudro@redhat.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting IPv4 packets 2021-04-21 9:27 ` Eelco Chaudron @ 2021-04-21 15:05 ` Davide Caratti 2021-04-22 9:17 ` Eelco Chaudron 0 siblings, 1 reply; 12+ messages in thread From: Davide Caratti @ 2021-04-21 15:05 UTC (permalink / raw) To: Eelco Chaudron Cc: Pravin B Shelar, David S. Miller, Jakub Kicinski, Sabrina Dubroca, netdev hello Eelco, thanks for looking at this! On Wed, 2021-04-21 at 11:27 +0200, Eelco Chaudron wrote: > > On 19 Apr 2021, at 17:23, Davide Caratti wrote: > > > running openvswitch on kernels built with KASAN, it's possible to see > > the > > following splat while testing fragmentation of IPv4 packets: > > <SNIP> > > > for IPv4 packets, ovs_fragment() uses a temporary struct dst_entry. > > Then, > > in the following call graph: > > > > ip_do_fragment() > > ip_skb_dst_mtu() > > ip_dst_mtu_maybe_forward() > > ip_mtu_locked() > > > > the pointer to struct dst_entry is used as pointer to struct rtable: > > this > > turns the access to struct members like rt_mtu_locked into an OOB read > > in > > the stack. Fix this changing the temporary variable used for IPv4 > > packets > > in ovs_fragment(), similarly to what is done for IPv6 few lines below. > > > > Fixes: d52e5a7e7ca4 ("ipv4: lock mtu in fnhe when received PMTU < > > net.ipv4.route.min_pmt") > > Cc: <stable@vger.kernel.org> > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > The fix looks good to me, however isn’t the real root cause > ip_mtu_locked() who casts struct dst_entry to struct rtable (not even > using container_of())? good point, that's my understanding (and the reason for that 'Fixes:' tag). Probably openvswitch was doing this on purpose, and it was "just working" until commit d52e5a7e7ca4. But at the current state, I see much easier to just fix the IPv4 part to have the same behavior as other "users" of ip_do_fragment(), like it happens for ovs_fragment() when the packet is IPv6 (or br_netfilter core, see [1]). By the way, apparently ip_do_fragment() already assumes that a struct rtable is available for the skb [2]. So, the fix in ovs_fragment() looks safer to me. WDYT? -- davide [1] https://elixir.bootlin.com/linux/v5.12-rc8/source/net/bridge/br_nf_core.c#L72 [2] https://elixir.bootlin.com/linux/v5.12-rc8/source/net/ipv4/ip_output.c#L778 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting IPv4 packets 2021-04-21 15:05 ` Davide Caratti @ 2021-04-22 9:17 ` Eelco Chaudron 0 siblings, 0 replies; 12+ messages in thread From: Eelco Chaudron @ 2021-04-22 9:17 UTC (permalink / raw) To: Davide Caratti Cc: Pravin B Shelar, David S. Miller, Jakub Kicinski, Sabrina Dubroca, netdev On 21 Apr 2021, at 17:05, Davide Caratti wrote: > hello Eelco, thanks for looking at this! > > On Wed, 2021-04-21 at 11:27 +0200, Eelco Chaudron wrote: >> >> On 19 Apr 2021, at 17:23, Davide Caratti wrote: >> >>> running openvswitch on kernels built with KASAN, it's possible to >>> see >>> the >>> following splat while testing fragmentation of IPv4 packets: >> >> <SNIP> >> >>> for IPv4 packets, ovs_fragment() uses a temporary struct dst_entry. >>> Then, >>> in the following call graph: >>> >>> ip_do_fragment() >>> ip_skb_dst_mtu() >>> ip_dst_mtu_maybe_forward() >>> ip_mtu_locked() >>> >>> the pointer to struct dst_entry is used as pointer to struct rtable: >>> this >>> turns the access to struct members like rt_mtu_locked into an OOB >>> read >>> in >>> the stack. Fix this changing the temporary variable used for IPv4 >>> packets >>> in ovs_fragment(), similarly to what is done for IPv6 few lines >>> below. >>> >>> Fixes: d52e5a7e7ca4 ("ipv4: lock mtu in fnhe when received PMTU < >>> net.ipv4.route.min_pmt") >>> Cc: <stable@vger.kernel.org> >>> Signed-off-by: Davide Caratti <dcaratti@redhat.com> >> >> The fix looks good to me, however isn’t the real root cause >> ip_mtu_locked() who casts struct dst_entry to struct rtable (not even >> using container_of())? > > good point, that's my understanding (and the reason for that 'Fixes:' > tag). Probably openvswitch was doing this on purpose, and it was "just > working" until commit d52e5a7e7ca4. > > But at the current state, I see much easier to just fix the IPv4 part > to > have the same behavior as other "users" of ip_do_fragment(), like it > happens for ovs_fragment() when the packet is IPv6 (or br_netfilter > core, see [1]). > > By the way, apparently ip_do_fragment() already assumes that a struct > rtable is available for the skb [2]. So, the fix in ovs_fragment() > looks > safer to me. WDYT? It looks like the assumption that a dst_entry is always embedded in rtable seems deeply embedded already, looking at skb_rtable(), so I agree this patch is the best solution. So again, Acked-by: Eelco Chaudron <echaudro@redhat.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets 2021-04-19 15:22 [PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets Davide Caratti 2021-04-19 15:23 ` [PATCH net 1/2] openvswitch: " Davide Caratti @ 2021-04-19 15:23 ` Davide Caratti 2021-04-19 17:38 ` Marcelo Ricardo Leitner ` (2 more replies) 2021-04-23 12:11 ` [PATCH net 0/2] " Davide Caratti 2 siblings, 3 replies; 12+ messages in thread From: Davide Caratti @ 2021-04-19 15:23 UTC (permalink / raw) To: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, wenxu, netdev Cc: Marcelo Ricardo Leitner when 'act_mirred' tries to fragment IPv4 packets that had been previously re-assembled using 'act_ct', splats like the following can be observed on kernels built with KASAN: BUG: KASAN: stack-out-of-bounds in ip_do_fragment+0x1b03/0x1f60 Read of size 1 at addr ffff888147009574 by task ping/947 CPU: 0 PID: 947 Comm: ping Not tainted 5.12.0-rc6+ #418 Hardware name: Red Hat KVM, BIOS 1.11.1-4.module+el8.1.0+4066+0f1aadab 04/01/2014 Call Trace: <IRQ> dump_stack+0x92/0xc1 print_address_description.constprop.7+0x1a/0x150 kasan_report.cold.13+0x7f/0x111 ip_do_fragment+0x1b03/0x1f60 sch_fragment+0x4bf/0xe40 tcf_mirred_act+0xc3d/0x11a0 [act_mirred] tcf_action_exec+0x104/0x3e0 fl_classify+0x49a/0x5e0 [cls_flower] tcf_classify_ingress+0x18a/0x820 __netif_receive_skb_core+0xae7/0x3340 __netif_receive_skb_one_core+0xb6/0x1b0 process_backlog+0x1ef/0x6c0 __napi_poll+0xaa/0x500 net_rx_action+0x702/0xac0 __do_softirq+0x1e4/0x97f do_softirq+0x71/0x90 </IRQ> __local_bh_enable_ip+0xdb/0xf0 ip_finish_output2+0x760/0x2120 ip_do_fragment+0x15a5/0x1f60 __ip_finish_output+0x4c2/0xea0 ip_output+0x1ca/0x4d0 ip_send_skb+0x37/0xa0 raw_sendmsg+0x1c4b/0x2d00 sock_sendmsg+0xdb/0x110 __sys_sendto+0x1d7/0x2b0 __x64_sys_sendto+0xdd/0x1b0 do_syscall_64+0x33/0x40 entry_SYSCALL_64_after_hwframe+0x44/0xae RIP: 0033:0x7f82e13853eb Code: 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 f3 0f 1e fa 48 8d 05 75 42 2c 00 41 89 ca 8b 00 85 c0 75 14 b8 2c 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 75 c3 0f 1f 40 00 41 57 4d 89 c7 41 56 41 89 RSP: 002b:00007ffe01fad888 EFLAGS: 00000246 ORIG_RAX: 000000000000002c RAX: ffffffffffffffda RBX: 00005571aac13700 RCX: 00007f82e13853eb RDX: 0000000000002330 RSI: 00005571aac13700 RDI: 0000000000000003 RBP: 0000000000002330 R08: 00005571aac10500 R09: 0000000000000010 R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe01faefb0 R13: 00007ffe01fad890 R14: 00007ffe01fad980 R15: 00005571aac0f0a0 The buggy address belongs to the page: page:000000001dff2e03 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x147009 flags: 0x17ffffc0001000(reserved) raw: 0017ffffc0001000 ffffea00051c0248 ffffea00051c0248 0000000000000000 raw: 0000000000000000 0000000000000000 00000001ffffffff 0000000000000000 page dumped because: kasan: bad access detected Memory state around the buggy address: ffff888147009400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffff888147009480: f1 f1 f1 f1 04 f2 f2 f2 f2 f2 f2 f2 00 00 00 00 >ffff888147009500: 00 00 00 00 00 00 00 00 00 00 f2 f2 f2 f2 f2 f2 ^ ffff888147009580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ffff888147009600: 00 00 00 00 00 00 00 00 00 00 00 00 00 f2 f2 f2 for IPv4 packets, sch_fragment() uses a temporary struct dst_entry. Then, in the following call graph: ip_do_fragment() ip_skb_dst_mtu() ip_dst_mtu_maybe_forward() ip_mtu_locked() the pointer to struct dst_entry is used as pointer to struct rtable: this turns the access to struct members like rt_mtu_locked into an OOB read in the stack. Fix this changing the temporary variable used for IPv4 packets in sch_fragment(), similarly to what is done for IPv6 few lines below. Fixes: c129412f74e9 ("net/sched: sch_frag: add generic packet fragment support.") Cc: <stable@vger.kernel.org> # 5.11 Reported-by: Shuang Li <shuali@redhat.com> Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/sched/sch_frag.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/net/sched/sch_frag.c b/net/sched/sch_frag.c index e1e77d3fb6c0..8c06381391d6 100644 --- a/net/sched/sch_frag.c +++ b/net/sched/sch_frag.c @@ -90,16 +90,16 @@ static int sch_fragment(struct net *net, struct sk_buff *skb, } if (skb_protocol(skb, true) == htons(ETH_P_IP)) { - struct dst_entry sch_frag_dst; + struct rtable sch_frag_rt = { 0 }; unsigned long orig_dst; sch_frag_prepare_frag(skb, xmit); - dst_init(&sch_frag_dst, &sch_frag_dst_ops, NULL, 1, + dst_init(&sch_frag_rt.dst, &sch_frag_dst_ops, NULL, 1, DST_OBSOLETE_NONE, DST_NOCOUNT); - sch_frag_dst.dev = skb->dev; + sch_frag_rt.dst.dev = skb->dev; orig_dst = skb->_skb_refdst; - skb_dst_set_noref(skb, &sch_frag_dst); + skb_dst_set_noref(skb, &sch_frag_rt.dst); IPCB(skb)->frag_max_size = mru; ret = ip_do_fragment(net, skb->sk, skb, sch_frag_xmit); -- 2.30.2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets 2021-04-19 15:23 ` [PATCH net 2/2] net/sched: sch_frag: " Davide Caratti @ 2021-04-19 17:38 ` Marcelo Ricardo Leitner 2021-04-19 18:46 ` Cong Wang 2021-04-20 19:27 ` Cong Wang 2 siblings, 0 replies; 12+ messages in thread From: Marcelo Ricardo Leitner @ 2021-04-19 17:38 UTC (permalink / raw) To: Davide Caratti Cc: Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller, Jakub Kicinski, wenxu, netdev On Mon, Apr 19, 2021 at 05:23:44PM +0200, Davide Caratti wrote: > when 'act_mirred' tries to fragment IPv4 packets that had been previously > re-assembled using 'act_ct', splats like the following can be observed on > kernels built with KASAN: Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets 2021-04-19 15:23 ` [PATCH net 2/2] net/sched: sch_frag: " Davide Caratti 2021-04-19 17:38 ` Marcelo Ricardo Leitner @ 2021-04-19 18:46 ` Cong Wang 2021-04-20 8:59 ` Davide Caratti 2021-04-20 19:27 ` Cong Wang 2 siblings, 1 reply; 12+ messages in thread From: Cong Wang @ 2021-04-19 18:46 UTC (permalink / raw) To: Davide Caratti Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, wenxu, Linux Kernel Network Developers, Marcelo Ricardo Leitner On Mon, Apr 19, 2021 at 8:24 AM Davide Caratti <dcaratti@redhat.com> wrote: > diff --git a/net/sched/sch_frag.c b/net/sched/sch_frag.c > index e1e77d3fb6c0..8c06381391d6 100644 > --- a/net/sched/sch_frag.c > +++ b/net/sched/sch_frag.c > @@ -90,16 +90,16 @@ static int sch_fragment(struct net *net, struct sk_buff *skb, > } > > if (skb_protocol(skb, true) == htons(ETH_P_IP)) { > - struct dst_entry sch_frag_dst; > + struct rtable sch_frag_rt = { 0 }; Is setting these fields 0 sufficient here? Because normally a struct table is initialized by rt_dst_alloc() which sets several of them to non-zero, notably, rt->rt_type and rt->rt_uncached. Similar for the IPv6 part, which is initialized by rt6_info_init(). Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets 2021-04-19 18:46 ` Cong Wang @ 2021-04-20 8:59 ` Davide Caratti 2021-04-20 19:25 ` Cong Wang 0 siblings, 1 reply; 12+ messages in thread From: Davide Caratti @ 2021-04-20 8:59 UTC (permalink / raw) To: Cong Wang Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, wenxu, Linux Kernel Network Developers, Marcelo Ricardo Leitner hello Cong, thanks for looking at this! On Mon, 2021-04-19 at 11:46 -0700, Cong Wang wrote: > On Mon, Apr 19, 2021 at 8:24 AM Davide Caratti <dcaratti@redhat.com> wrote: > > diff --git a/net/sched/sch_frag.c b/net/sched/sch_frag.c > > index e1e77d3fb6c0..8c06381391d6 100644 > > --- a/net/sched/sch_frag.c > > +++ b/net/sched/sch_frag.c > > @@ -90,16 +90,16 @@ static int sch_fragment(struct net *net, struct sk_buff *skb, > > } > > > > if (skb_protocol(skb, true) == htons(ETH_P_IP)) { > > - struct dst_entry sch_frag_dst; > > + struct rtable sch_frag_rt = { 0 }; > > Is setting these fields 0 sufficient here? Because normally a struct table > is initialized by rt_dst_alloc() which sets several of them to non-zero, > notably, rt->rt_type and rt->rt_uncached. > > Similar for the IPv6 part, which is initialized by rt6_info_init(). for what we do now in openvswitch and sch_frag, that should be sufficient: a similar thing is done by br_netfilter [1], apparently for the same "refragmentation" purposes. On a fedora host (running 5.10, but it shouldn't be much different than current Linux), I just dumped 'fake_rtable' from a bridge device: # ip link add name test-br0 type bridge # ip link set dev test-br0 up # ip link add name test-port0 type dummy # ip link set dev test-port0 master test-br0 up # crash [...] crash> net NET_DEVICE NAME IP ADDRESS(ES) [...] ffff89fb44ed8000 test-br0 ffff89fbfc45c000 test-port0 crash> p sizeof(struct net_device) $12 = 3200 crash> p ((struct net_bridge*)(0xffff89fb44ed8000 + 3200))->fake_rtable $13 = { dst = { dev = 0xffff89fb44ed8000, ops = 0xffffffffc0afef40, _metrics = 18446744072647256257, expires = 0, xfrm = 0x0, input = 0x0, output = 0x0, flags = 18, <-- that should be DST_NOXFRM | DST_FAKE_RTABLE obsolete = 0, header_len = 0, trailer_len = 0, __refcnt = { counter = 1 }, __use = 0, lastuse = 0, lwtstate = 0x0, callback_head = { next = 0x0, func = 0x0 }, error = 0, __pad = 0, tclassid = 0 }, rt_genid = 0, rt_flags = 0, rt_type = 0, rt_is_input = 0 '\000', rt_uses_gateway = 0 '\000', rt_iif = 0, rt_gw_family = 0 '\000', { rt_gw4 = 0, rt_gw6 = { in6_u = { u6_addr8 = "\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000", u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, u6_addr32 = {0, 0, 0, 0} } } }, rt_mtu_locked = 0, rt_pmtu = 0, rt_uncached = { next = 0x0, prev = 0x0 }, rt_uncached_list = 0x0 } only fake_rtable.dst members are set to something, the remaining are all zero-ed. -- davide [1] https://elixir.bootlin.com/linux/latest/source/net/bridge/br_nf_core.c#L62 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets 2021-04-20 8:59 ` Davide Caratti @ 2021-04-20 19:25 ` Cong Wang 0 siblings, 0 replies; 12+ messages in thread From: Cong Wang @ 2021-04-20 19:25 UTC (permalink / raw) To: Davide Caratti Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, wenxu, Linux Kernel Network Developers, Marcelo Ricardo Leitner On Tue, Apr 20, 2021 at 1:59 AM Davide Caratti <dcaratti@redhat.com> wrote: > > hello Cong, thanks for looking at this! > > On Mon, 2021-04-19 at 11:46 -0700, Cong Wang wrote: > > On Mon, Apr 19, 2021 at 8:24 AM Davide Caratti <dcaratti@redhat.com> wrote: > > > diff --git a/net/sched/sch_frag.c b/net/sched/sch_frag.c > > > index e1e77d3fb6c0..8c06381391d6 100644 > > > --- a/net/sched/sch_frag.c > > > +++ b/net/sched/sch_frag.c > > > @@ -90,16 +90,16 @@ static int sch_fragment(struct net *net, struct sk_buff *skb, > > > } > > > > > > if (skb_protocol(skb, true) == htons(ETH_P_IP)) { > > > - struct dst_entry sch_frag_dst; > > > + struct rtable sch_frag_rt = { 0 }; > > > > Is setting these fields 0 sufficient here? Because normally a struct table > > is initialized by rt_dst_alloc() which sets several of them to non-zero, > > notably, rt->rt_type and rt->rt_uncached. > > > > Similar for the IPv6 part, which is initialized by rt6_info_init(). > > for what we do now in openvswitch and sch_frag, that should be > sufficient: a similar thing is done by br_netfilter [1], apparently for > the same "refragmentation" purposes. On a fedora host (running 5.10, but > it shouldn't be much different than current Linux), I just dumped > 'fake_rtable' from a bridge device: Sounds fair. It looks like all of these cases merely use dst->dev->mtu, so it is overkill to pass a full dst just to satisfy some callees. Anyway, your patch should be okay as a fix for -net. Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets 2021-04-19 15:23 ` [PATCH net 2/2] net/sched: sch_frag: " Davide Caratti 2021-04-19 17:38 ` Marcelo Ricardo Leitner 2021-04-19 18:46 ` Cong Wang @ 2021-04-20 19:27 ` Cong Wang 2 siblings, 0 replies; 12+ messages in thread From: Cong Wang @ 2021-04-20 19:27 UTC (permalink / raw) To: Davide Caratti Cc: Jamal Hadi Salim, Jiri Pirko, David S. Miller, Jakub Kicinski, wenxu, Linux Kernel Network Developers, Marcelo Ricardo Leitner On Mon, Apr 19, 2021 at 8:24 AM Davide Caratti <dcaratti@redhat.com> wrote: > > when 'act_mirred' tries to fragment IPv4 packets that had been previously > re-assembled using 'act_ct', splats like the following can be observed on > kernels built with KASAN: [...] > for IPv4 packets, sch_fragment() uses a temporary struct dst_entry. Then, > in the following call graph: > > ip_do_fragment() > ip_skb_dst_mtu() > ip_dst_mtu_maybe_forward() > ip_mtu_locked() > > the pointer to struct dst_entry is used as pointer to struct rtable: this > turns the access to struct members like rt_mtu_locked into an OOB read in > the stack. Fix this changing the temporary variable used for IPv4 packets > in sch_fragment(), similarly to what is done for IPv6 few lines below. > > Fixes: c129412f74e9 ("net/sched: sch_frag: add generic packet fragment support.") > Cc: <stable@vger.kernel.org> # 5.11 > Reported-by: Shuang Li <shuali@redhat.com> > Signed-off-by: Davide Caratti <dcaratti@redhat.com> Acked-by: Cong Wang <cong.wang@bytedance.com> Thanks. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets 2021-04-19 15:22 [PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets Davide Caratti 2021-04-19 15:23 ` [PATCH net 1/2] openvswitch: " Davide Caratti 2021-04-19 15:23 ` [PATCH net 2/2] net/sched: sch_frag: " Davide Caratti @ 2021-04-23 12:11 ` Davide Caratti 2 siblings, 0 replies; 12+ messages in thread From: Davide Caratti @ 2021-04-23 12:11 UTC (permalink / raw) To: netdev, David S. Miller, Jakub Kicinski On Mon, 2021-04-19 at 17:22 +0200, Davide Caratti wrote: > - patch 1/2 fixes openvswitch IPv4 fragmentation, that does a stack OOB > read after commit d52e5a7e7ca4 ("ipv4: lock mtu in fnhe when received > PMTU < net.ipv4.route.min_pmt") > - patch 2/2 fixes the same issue in TC 'sch_frag' code > hello Dave and Jakub, I see that patches in this series [1][2] are marked with 'Changes Requested' in patchwork, but in my understanding no further changes are requested for the moment. do I need to re-send the series, or you can manage to change the status inside patchwork? thanks, -- davide [1] https://patchwork.kernel.org/project/netdevbpf/patch/94839fa9e7995afa6139b4f65c12ac15c1a8dc2f.1618844973.git.dcaratti@redhat.com/ [2] https://patchwork.kernel.org/project/netdevbpf/patch/80dbe764b5ae660bba3cf6edcb045a74b0f85853.1618844973.git.dcaratti@redhat.com/ ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-04-23 12:11 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-19 15:22 [PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets Davide Caratti 2021-04-19 15:23 ` [PATCH net 1/2] openvswitch: " Davide Caratti 2021-04-21 9:27 ` Eelco Chaudron 2021-04-21 15:05 ` Davide Caratti 2021-04-22 9:17 ` Eelco Chaudron 2021-04-19 15:23 ` [PATCH net 2/2] net/sched: sch_frag: " Davide Caratti 2021-04-19 17:38 ` Marcelo Ricardo Leitner 2021-04-19 18:46 ` Cong Wang 2021-04-20 8:59 ` Davide Caratti 2021-04-20 19:25 ` Cong Wang 2021-04-20 19:27 ` Cong Wang 2021-04-23 12:11 ` [PATCH net 0/2] " Davide Caratti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).