| Message ID | 20210112004831.3778323-1-olteanv@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: dsa: clear devlink port type before unregistering slave netdevs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On 1/11/21 4:48 PM, Vladimir Oltean wrote: > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > Florian reported a use-after-free bug in devlink_nl_port_fill found with > KASAN: > > (devlink_nl_port_fill) > (devlink_port_notify) > (devlink_port_unregister) > (dsa_switch_teardown.part.3) > (dsa_tree_teardown_switches) > (dsa_unregister_switch) > (bcm_sf2_sw_remove) > (platform_remove) > (device_release_driver_internal) > (device_links_unbind_consumers) > (device_release_driver_internal) > (device_driver_detach) > (unbind_store) > > Allocated by task 31: > alloc_netdev_mqs+0x5c/0x50c > dsa_slave_create+0x110/0x9c8 > dsa_register_switch+0xdb0/0x13a4 > b53_switch_register+0x47c/0x6dc > bcm_sf2_sw_probe+0xaa4/0xc98 > platform_probe+0x90/0xf4 > really_probe+0x184/0x728 > driver_probe_device+0xa4/0x278 > __device_attach_driver+0xe8/0x148 > bus_for_each_drv+0x108/0x158 > > Freed by task 249: > free_netdev+0x170/0x194 > dsa_slave_destroy+0xac/0xb0 > dsa_port_teardown.part.2+0xa0/0xb4 > dsa_tree_teardown_switches+0x50/0xc4 > dsa_unregister_switch+0x124/0x250 > bcm_sf2_sw_remove+0x98/0x13c > platform_remove+0x44/0x5c > device_release_driver_internal+0x150/0x254 > device_links_unbind_consumers+0xf8/0x12c > device_release_driver_internal+0x84/0x254 > device_driver_detach+0x30/0x34 > unbind_store+0x90/0x134 > > What happens is that devlink_port_unregister emits a netlink > DEVLINK_CMD_PORT_DEL message which associates the devlink port that is > getting unregistered with the ifindex of its corresponding net_device. > Only trouble is, the net_device has already been unregistered. > > It looks like we can stub out the search for a corresponding net_device > if we clear the devlink_port's type. This looks like a bit of a hack, > but also seems to be the reason why the devlink_port_type_clear function > exists in the first place. > > Fixes: 3122433eb533 ("net: dsa: Register devlink ports before calling DSA driver setup()") > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Tested-by: Florian fainelli <f.fainelli@gmail.com> Reported-by: Florian Fainelli <f.fainelli@gmail.com> Thanks!
On Tue, 12 Jan 2021 10:49:21 -0800 Florian Fainelli wrote: > On 1/11/21 4:48 PM, Vladimir Oltean wrote: > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > > > Florian reported a use-after-free bug in devlink_nl_port_fill found with > > KASAN: > > > > (devlink_nl_port_fill) > > (devlink_port_notify) > > (devlink_port_unregister) > > (dsa_switch_teardown.part.3) > > (dsa_tree_teardown_switches) > > (dsa_unregister_switch) > > (bcm_sf2_sw_remove) > > (platform_remove) > > (device_release_driver_internal) > > (device_links_unbind_consumers) > > (device_release_driver_internal) > > (device_driver_detach) > > (unbind_store) > > > > Allocated by task 31: > > alloc_netdev_mqs+0x5c/0x50c > > dsa_slave_create+0x110/0x9c8 > > dsa_register_switch+0xdb0/0x13a4 > > b53_switch_register+0x47c/0x6dc > > bcm_sf2_sw_probe+0xaa4/0xc98 > > platform_probe+0x90/0xf4 > > really_probe+0x184/0x728 > > driver_probe_device+0xa4/0x278 > > __device_attach_driver+0xe8/0x148 > > bus_for_each_drv+0x108/0x158 > > > > Freed by task 249: > > free_netdev+0x170/0x194 > > dsa_slave_destroy+0xac/0xb0 > > dsa_port_teardown.part.2+0xa0/0xb4 > > dsa_tree_teardown_switches+0x50/0xc4 > > dsa_unregister_switch+0x124/0x250 > > bcm_sf2_sw_remove+0x98/0x13c > > platform_remove+0x44/0x5c > > device_release_driver_internal+0x150/0x254 > > device_links_unbind_consumers+0xf8/0x12c > > device_release_driver_internal+0x84/0x254 > > device_driver_detach+0x30/0x34 > > unbind_store+0x90/0x134 > > > > What happens is that devlink_port_unregister emits a netlink > > DEVLINK_CMD_PORT_DEL message which associates the devlink port that is > > getting unregistered with the ifindex of its corresponding net_device. > > Only trouble is, the net_device has already been unregistered. > > > > It looks like we can stub out the search for a corresponding net_device > > if we clear the devlink_port's type. This looks like a bit of a hack, > > but also seems to be the reason why the devlink_port_type_clear function > > exists in the first place. > > > > Fixes: 3122433eb533 ("net: dsa: Register devlink ports before calling DSA driver setup()") > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > Tested-by: Florian fainelli <f.fainelli@gmail.com> > Reported-by: Florian Fainelli <f.fainelli@gmail.com> Applied, thanks!
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index c05b1b54c4f7..839c3a770d23 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -353,9 +353,13 @@ static int dsa_port_devlink_setup(struct dsa_port *dp) static void dsa_port_teardown(struct dsa_port *dp) { + struct devlink_port *dlp = &dp->devlink_port; + if (!dp->setup) return; + devlink_port_type_clear(dlp); + switch (dp->type) { case DSA_PORT_TYPE_UNUSED: break;