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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <36693a9a56f1054f55b42b1a25a4c70d3dbb1728.camel@redhat.com>
Date:   Wed, 21 Apr 2021 17:05:18 +0200
From:   Davide Caratti <dcaratti@...hat.com>
To:     Eelco Chaudron <echaudro@...hat.com>
Cc:     Pravin B Shelar <pshelar@....org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Sabrina Dubroca <sd@...asysnail.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net 1/2] openvswitch: fix stack OOB read while
 fragmenting IPv4 packets

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@...r.kernel.org>
> > Signed-off-by: Davide Caratti <dcaratti@...hat.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