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 <[email protected]>
---
drivers/net/ethernet/freescale/ucc_geth.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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;
}
--
2.23.0
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?
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
On Sat, 5 Dec 2020 22:04:28 +0100 Rasmus Villemoes wrote:
> 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 was concerned about the fact that we free things and release
references while the device may still be up (given that it's
unregister_netdev() that will take it down).
> I only noticed because I needed to add a free of the ug_info in a later
> patch.
On 05/12/2020 22.19, Jakub Kicinski wrote:
> On Sat, 5 Dec 2020 22:04:28 +0100 Rasmus Villemoes wrote:
>> 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 was concerned about the fact that we free things and release
> references while the device may still be up (given that it's
> unregister_netdev() that will take it down).
I guess you're right. I'll fix it locally (and pull the patch earlier)
and wait a few days with sending an updated version to give Li Yang some
time to say if he wants to handle the series or not.
Thanks,
Rasmus
> I only noticed because I needed to add a free of the ug_info in a later
> patch.
Where, ironically, I add a use-after-free bug by freeing ug_info before
the ucc_geth_memclean() call.
:facepalm: