summaryrefslogtreecommitdiffstats
path: root/net
diff options
context:
space:
mode:
authorVladimir Oltean <vladimir.oltean@nxp.com>2020-01-25 23:01:11 +0200
committerDavid S. Miller <davem@davemloft.net>2020-01-27 11:12:46 +0100
commit6dc43cd3aae0ffeeaebe427e1bf5e5faf9de7d42 (patch)
tree2401a5c5701ccede993d4e12048be7d8295493a6 /net
parenta85dd3a5170c8812cd835ea968ccadf0ebf1648e (diff)
downloadlinux-6dc43cd3aae0ffeeaebe427e1bf5e5faf9de7d42.tar.bz2
net: dsa: Fix use-after-free in probing of DSA switch tree
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): [ 5.477947] dsa_tree_setup_switches: Setting up port 0 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.313002] dsa_tree_setup_switches: Setting up port 1 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.319932] dsa_tree_setup_switches: Setting up port 2 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.329693] dsa_tree_setup_switches: Setting up port 3 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.339458] dsa_tree_setup_switches: Setting up port 4 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.349226] dsa_tree_setup_switches: Setting up port 5 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.358991] dsa_tree_setup_switches: Setting up port 6 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.368758] dsa_tree_setup_switches: Setting up port 7 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.378524] dsa_tree_setup_switches: Setting up port 8 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.388291] dsa_tree_setup_switches: Setting up port 9 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.398057] dsa_tree_setup_switches: Setting up port 10 of switch ffffff803df0b980 (dev ffffff803f775c00) [ 6.407912] dsa_tree_setup_switches: Setting up port 0 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.417682] dsa_tree_setup_switches: Setting up port 1 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.427446] dsa_tree_setup_switches: Setting up port 2 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.437212] dsa_tree_setup_switches: Setting up port 3 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.446979] dsa_tree_setup_switches: Setting up port 4 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.456744] dsa_tree_setup_switches: Setting up port 5 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.466512] dsa_tree_setup_switches: Setting up port 6 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.476277] dsa_tree_setup_switches: Setting up port 7 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.486043] dsa_tree_setup_switches: Setting up port 8 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.495810] dsa_tree_setup_switches: Setting up port 9 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.505577] dsa_tree_setup_switches: Setting up port 10 of switch ffffff803da02f80 (dev 0000000000000000) [ 6.515433] dsa_tree_setup_switches: Setting up port 0 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.354120] dsa_tree_setup_switches: Setting up port 1 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.361045] dsa_tree_setup_switches: Setting up port 2 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.370805] dsa_tree_setup_switches: Setting up port 3 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.380571] dsa_tree_setup_switches: Setting up port 4 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.390337] dsa_tree_setup_switches: Setting up port 5 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.400104] dsa_tree_setup_switches: Setting up port 6 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.409872] dsa_tree_setup_switches: Setting up port 7 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.419637] dsa_tree_setup_switches: Setting up port 8 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.429403] dsa_tree_setup_switches: Setting up port 9 of switch ffffff803db15b80 (dev ffffff803d8e4800) [ 7.439169] dsa_tree_setup_switches: Setting up port 10 of switch ffffff803db15b80 (dev ffffff803d8e4800) 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@nxp.com> Signed-off-by: David S. Miller <davem@davemloft.net>
Diffstat (limited to 'net')
-rw-r--r--net/dsa/dsa2.c36
1 files changed, 25 insertions, 11 deletions
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index c6d81f2baf4e..e7c30b472034 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -849,6 +849,19 @@ static int dsa_switch_parse(struct dsa_switch *ds, struct dsa_chip_data *cd)
return dsa_switch_parse_ports(ds, cd);
}
+static void dsa_switch_release_ports(struct dsa_switch *ds)
+{
+ struct dsa_switch_tree *dst = ds->dst;
+ struct dsa_port *dp, *next;
+
+ list_for_each_entry_safe(dp, next, &dst->ports, list) {
+ if (dp->ds != ds)
+ continue;
+ list_del(&dp->list);
+ kfree(dp);
+ }
+}
+
static int dsa_switch_probe(struct dsa_switch *ds)
{
struct dsa_switch_tree *dst;
@@ -865,12 +878,17 @@ static int dsa_switch_probe(struct dsa_switch *ds)
if (!ds->num_ports)
return -EINVAL;
- if (np)
+ if (np) {
err = dsa_switch_parse_of(ds, np);
- else if (pdata)
+ if (err)
+ dsa_switch_release_ports(ds);
+ } else if (pdata) {
err = dsa_switch_parse(ds, pdata);
- else
+ if (err)
+ dsa_switch_release_ports(ds);
+ } else {
err = -ENODEV;
+ }
if (err)
return err;
@@ -878,8 +896,10 @@ static int dsa_switch_probe(struct dsa_switch *ds)
dst = ds->dst;
dsa_tree_get(dst);
err = dsa_tree_setup(dst);
- if (err)
+ if (err) {
+ dsa_switch_release_ports(ds);
dsa_tree_put(dst);
+ }
return err;
}
@@ -900,15 +920,9 @@ EXPORT_SYMBOL_GPL(dsa_register_switch);
static void dsa_switch_remove(struct dsa_switch *ds)
{
struct dsa_switch_tree *dst = ds->dst;
- struct dsa_port *dp, *next;
dsa_tree_teardown(dst);
-
- list_for_each_entry_safe(dp, next, &dst->ports, list) {
- list_del(&dp->list);
- kfree(dp);
- }
-
+ dsa_switch_release_ports(ds);
dsa_tree_put(dst);
}