[PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets

[PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets

[Davide Caratti]

- 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

[PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting IPv4 packets

[Davide Caratti]

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

[PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets

[Davide Caratti]

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

Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets

[Marcelo Ricardo Leitner]

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>

Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets

[Cong Wang]

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.

Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets

[Davide Caratti]

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

Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets

[Cong Wang]

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.

Re: [PATCH net 2/2] net/sched: sch_frag: fix stack OOB read while fragmenting IPv4 packets

[Cong Wang]

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.

Re: [PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting IPv4 packets

[Eelco Chaudron]

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>

Re: [PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting IPv4 packets

[Davide Caratti]

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

Re: [PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting IPv4 packets

[Eelco Chaudron]

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>

Re: [PATCH net 0/2] fix stack OOB read while fragmenting IPv4 packets

[Davide Caratti]

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/