Message ID | 20200125210111.22058-1-olteanv@gmail.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | [net] net: dsa: Fix use-after-free in probing of DSA switch tree | expand |
From: Vladimir Oltean <olteanv@gmail.com> Date: Sat, 25 Jan 2020 23:01:11 +0200 > From: Vladimir Oltean <vladimir.oltean@nxp.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@nxp.com> Applied, thank you.
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); }