| Message ID | 20201205191744.7847-12-rasmus.villemoes@prevas.dk |
|---|---|
| State | New |
| Headers | show |
| Series | [01/20] ethernet: ucc_geth: set dev->max_mtu to 1518 | expand |
On 05/12/2020 21.48, Jakub Kicinski wrote: > On Sat, 5 Dec 2020 20:17:34 +0100 Rasmus Villemoes wrote: >> - unregister_netdev(dev); >> - free_netdev(dev); >> ucc_geth_memclean(ugeth); >> if (of_phy_is_fixed_link(np)) >> of_phy_deregister_fixed_link(np); >> of_node_put(ugeth->ug_info->tbi_node); >> of_node_put(ugeth->ug_info->phy_node); >> + unregister_netdev(dev); >> + free_netdev(dev); > > Are you sure you want to move the unregister_netdev() as well as the > free? > Hm, dunno, I don't think it's needed per se, but it also shouldn't hurt from what I can tell. It seems more natural that they go together, but if you prefer a minimal patch that's of course also possible. I only noticed because I needed to add a free of the ug_info in a later patch. Rasmus
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index b132fcfc7c17..ba911d05d36d 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -3895,13 +3895,13 @@ static int ucc_geth_remove(struct platform_device* ofdev) struct ucc_geth_private *ugeth = netdev_priv(dev); struct device_node *np = ofdev->dev.of_node; - unregister_netdev(dev); - free_netdev(dev); ucc_geth_memclean(ugeth); if (of_phy_is_fixed_link(np)) of_phy_deregister_fixed_link(np); of_node_put(ugeth->ug_info->tbi_node); of_node_put(ugeth->ug_info->phy_node); + unregister_netdev(dev); + free_netdev(dev); return 0; }
ugeth is the netdiv_priv() part of the netdevice. Accessing the memory pointed to by ugeth (such as done by ucc_geth_memclean() and the two of_node_puts) after free_netdev() is thus use-after-free. Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/net/ethernet/freescale/ucc_geth.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)