netdev - Re: [PATCH net] net: dsa: Fix use-after-free in probing of DSA switch tree

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] [day] [month] [year] [list]

Message-Id: <20200127.111327.391981642587628280.davem@davemloft.net>
Date:   Mon, 27 Jan 2020 11:13:27 +0100 (CET)
From:   David Miller <davem@...emloft.net>
To:     olteanv@...il.com
Cc:     f.fainelli@...il.com, vivien.didelot@...il.com, andrew@...n.ch,
        netdev@...r.kernel.org, marek.behun@....cz, vladimir.oltean@....com
Subject: Re: [PATCH net] net: dsa: Fix use-after-free in probing of DSA
 switch tree

From: Vladimir Oltean <olteanv@...il.com>
Date: Sat, 25 Jan 2020 23:01:11 +0200

> From: Vladimir Oltean <vladimir.oltean@....com>
> 
> DSA sets up a switch tree little by little. Every switch of the N
> members of the tree calls dsa_register_switch, and (N - 1) will just
> touch the dst->ports list with their ports and quickly exit. Only the
> last switch that calls dsa_register_switch will find all DSA links
> complete in dsa_tree_setup_routing_table, and not return zero as a
> result but instead go ahead and set up the entire DSA switch tree
> (practically on behalf of the other switches too).
> 
> The trouble is that the (N - 1) switches don't clean up after themselves
> after they get an error such as EPROBE_DEFER. Their footprint left in
> dst->ports by dsa_switch_touch_ports is still there. And switch N, the
> one responsible with actually setting up the tree, is going to work with
> those stale dp, dp->ds and dp->ds->dev pointers. In particular ds and
> ds->dev might get freed by the device driver.
> 
> Be there a 2-switch tree and the following calling order:
> - Switch 1 calls dsa_register_switch
>   - Calls dsa_switch_touch_ports, populates dst->ports
>   - Calls dsa_port_parse_cpu, gets -EPROBE_DEFER, exits.
> - Switch 2 calls dsa_register_switch
>   - Calls dsa_switch_touch_ports, populates dst->ports
>   - Probe doesn't get deferred, so it goes ahead.
>   - Calls dsa_tree_setup_routing_table, which returns "complete == true"
>     due to Switch 1 having called dsa_switch_touch_ports before.
>   - Because the DSA links are complete, it calls dsa_tree_setup_switches
>     now.
>   - dsa_tree_setup_switches iterates through dst->ports, initializing
>     the Switch 1 ds structure (invalid) and the Switch 2 ds structure
>     (valid).
>   - Undefined behavior (use after free, sometimes NULL pointers, etc).
> 
> Real example below (debugging prints added by me, as well as guards
> against NULL pointers):
 ...
> The solution is to recognize that the functions that call
> dsa_switch_touch_ports (dsa_switch_parse_of, dsa_switch_parse) have side
> effects, and therefore one should clean up their side effects on error
> path. The cleanup of dst->ports was taken from dsa_switch_remove and
> moved into a dedicated dsa_switch_release_ports function, which should
> really be per-switch (free only the members of dst->ports that are also
> members of ds, instead of all switch ports).
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>

Applied, thank you.