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: <1097839A-30AD-4AE9-859A-4B7C6A3EFA40@redhat.com>
Date:   Wed, 21 Apr 2021 11:27:21 +0200
From:   "Eelco Chaudron" <echaudro@...hat.com>
To:     "Davide Caratti" <dcaratti@...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
Subject: Re: [PATCH net 1/2] openvswitch: fix stack OOB read while fragmenting
 IPv4 packets



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())?

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@...hat.com>