| Message ID | 29c95029f83aa44bcbdb5a314cb700e077df2291.1618604533.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: sch_frag: fix OOB read while processing IPv4 fragments | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 1 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 20 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1 this patch: 1 |
| netdev/header_inline | success | Link |
On Fri, 2021-04-16 at 22:29 +0200, Davide Caratti wrote: [...] > > for IPv4 packets, sch_fragment() uses a temporary struct dst_entry. Then, > in the following call graph: > > ip_fragment() ^^ the above line is a typo, > ip_do_fragment() > ip_skb_dst_mtu() > ip_dst_mtu_maybe_forward() > ip_mtu_locked() > > a pointer to that struct is casted as pointer to struct rtable, hence the > OOB stack access. Fix this, changing the temporary variable used for IPv4 > packets in sch_fragment(), similarly to what is done for IPv6 in the same > function. and thanks to Eelco's help I just reproduced a similar splat with openvswitch. Indeed, ovs_fragment() seems to have the same problem [1]; I will follow-up with a series that fixes both data-paths.
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);
when the Linux kernel fragments a packet that was previously re-assembled by the 'act_ct' action, the following splat can be seen on KASAN kernels: BUG: KASAN: stack-out-of-bounds in ip_do_fragment+0x1b03/0x1f60 Read of size 1 at addr ffff88887f209574 by task ping/5640 CPU: 29 PID: 5640 Comm: ping Tainted: G S 5.12.0-rc6+ #413 Hardware name: Supermicro SYS-6027R-72RF/X9DRH-7TF/7F/iTF/iF, BIOS 3.0 07/26/2013 Call Trace: <IRQ> dump_stack+0x92/0xc1 print_address_description.constprop.7+0x1a/0x150 kasan_report.cold.17+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] for IPv4 packets, sch_fragment() uses a temporary struct dst_entry. Then, in the following call graph: ip_fragment() ip_do_fragment() ip_skb_dst_mtu() ip_dst_mtu_maybe_forward() ip_mtu_locked() a pointer to that struct is casted as pointer to struct rtable, hence the OOB stack access. Fix this, changing the temporary variable used for IPv4 packets in sch_fragment(), similarly to what is done for IPv6 in the same function. Fixes: c129412f74e9 ("net/sched: sch_frag: add generic packet fragment support.") 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(-)