[v2] net: eth-uclass: revalidate priv after stop() in eth_halt()

In eth_halt(), reread and revalidate priv after calling stop(),
as it may have been freed, leaving a dangling pointer.

In the ethernet gadget implementation, the gadget device gets
probed during start() and removed during stop(), which includes
freeing `uclass_priv_` to which `priv` is pointing. Writing to
`priv` after stop() may corrupt the `fd` member of `struct
malloc_chunk`, which represents the freed block, and could cause
hard-to-debug crashes on subsequent calls to malloc()/free().

Signed-off-by: Niel Fourie <lusus@denx.de>
Cc: Ramon Fried <rfried.dev@gmail.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Revalidate priv instead of changing state before stop()
- Added explanational comment

This patch my be dropped if the patch which addresses the root cause
("usb: gadget: ether: split start/stop from init/halt") is accepted.

 net/eth-uclass.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

On 12/12/22 16:29, Niel Fourie wrote:
> In eth_halt(), reread and revalidate priv after calling stop(),
> as it may have been freed, leaving a dangling pointer.
> 
> In the ethernet gadget implementation, the gadget device gets
> probed during start() and removed during stop(), which includes
> freeing `uclass_priv_` to which `priv` is pointing. Writing to
> `priv` after stop() may corrupt the `fd` member of `struct
> malloc_chunk`, which represents the freed block, and could cause
> hard-to-debug crashes on subsequent calls to malloc()/free().
> 
> Signed-off-by: Niel Fourie <lusus@denx.de>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Lukasz Majewski <lukma@denx.de>
> ---
> Changes for v2:
> - Revalidate priv instead of changing state before stop()
> - Added explanational comment
> 
> This patch my be dropped if the patch which addresses the root cause
> ("usb: gadget: ether: split start/stop from init/halt") is accepted.
> 
>   net/eth-uclass.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index f41da4b37b3..7d5783b5cab 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -341,8 +341,11 @@ void eth_halt(void)
>   	priv = dev_get_uclass_priv(current);
>   	if (!priv || !priv->running)
>   		return;
> -
>   	eth_get_ops(current)->stop(current);
> +	/* Ethernet gadget frees priv during stop, workaround until fixed... */
> +	priv = dev_get_uclass_priv(current);
> +	if (!priv || !priv->running)
> +		return;

Is this fixed by the second patch in this series ?
If so, drop this patch.

On 12/12/22 16:29, Niel Fourie wrote:
> In eth_halt(), reread and revalidate priv after calling stop(),
> as it may have been freed, leaving a dangling pointer.
> 
> In the ethernet gadget implementation, the gadget device gets
> probed during start() and removed during stop(), which includes
> freeing `uclass_priv_` to which `priv` is pointing. Writing to
> `priv` after stop() may corrupt the `fd` member of `struct
> malloc_chunk`, which represents the freed block, and could cause
> hard-to-debug crashes on subsequent calls to malloc()/free().
> 
> Signed-off-by: Niel Fourie <lusus@denx.de>
> Cc: Ramon Fried <rfried.dev@gmail.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Lukasz Majewski <lukma@denx.de>
> ---
> Changes for v2:
> - Revalidate priv instead of changing state before stop()
> - Added explanational comment
> 
> This patch my be dropped if the patch which addresses the root cause
> ("usb: gadget: ether: split start/stop from init/halt") is accepted.
> 
>   net/eth-uclass.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index f41da4b37b3..7d5783b5cab 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -341,8 +341,11 @@ void eth_halt(void)
>   	priv = dev_get_uclass_priv(current);
>   	if (!priv || !priv->running)
>   		return;
> -

This shouldn't be here.

>   	eth_get_ops(current)->stop(current);
> +	/* Ethernet gadget frees priv during stop, workaround until fixed... */
> +	priv = dev_get_uclass_priv(current);
> +	if (!priv || !priv->running)
> +		return;
>   	priv->state = ETH_STATE_PASSIVE;
>   	priv->running = false;
>   }

My understanding of discussion in
[PATCH] usb: gadget: ether: split start/stop from init/halt
is that this patch is not needed with the aforementioned one applied.