netdev - Re: [PATCH net v2] stmmac: Clear variable when destroying workqueue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zdy04YvIFlkOl3Z-@nanopsycho>
Date: Mon, 26 Feb 2024 16:57:21 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Raczynski <j.raczynski@...sung.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, alexandre.torgue@...s.st.com,
	joabreu@...opsys.com
Subject: Re: [PATCH net v2] stmmac: Clear variable when destroying workqueue

Mon, Feb 26, 2024 at 04:42:17PM CET, j.raczynski@...sung.com wrote:
>Currently when suspending driver and stopping workqueue it is checked whether
>workqueue is not NULL and if so, it is destroyed.
>Function destroy_workqueue() does drain queue and does clear variable, but
>it does not set workqueue variable to NULL. This can cause kernel/module
>panic if code attempts to clear workqueue that was not initialized.
>
>This scenario is possible when resuming suspended driver in stmmac_resume(),
>because there is no handling for failed stmmac_hw_setup(),
>which can fail and return if DMA engine has failed to initialize,
>and workqueue is initialized after DMA engine.
>Should DMA engine fail to initialize, resume will proceed normally,
>but interface won't work and TX queue will eventually timeout,
>causing 'Reset adapter' error.
>This then does destroy workqueue during reset process.
>And since workqueue is initialized after DMA engine and can be skipped,
>it will cause kernel/module panic.

If you have a trace, it is good to inline it here so the future
reader/backporter can immediately match it.

>
>This commit sets workqueue variable to NULL when destroying workqueue,

Don't talk about "this commit" in the patch description, just tell the
codebase what to do using imperative mood:
https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes


>which secures against that possible driver crash.
>
>Fixes: 5a5586112b929 ("net: stmmac: support FPE link partner hand-shaking procedure")
>Signed-off-by: Jakub Raczynski <j.raczynski@...sung.com>
>---
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>index 75d029704503..0681029a2489 100644
>--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>@@ -4005,8 +4005,10 @@ static void stmmac_fpe_stop_wq(struct stmmac_priv *priv)
> {
> 	set_bit(__FPE_REMOVING, &priv->fpe_task_state);
> 
>-	if (priv->fpe_wq)
>+	if (priv->fpe_wq) {
> 		destroy_workqueue(priv->fpe_wq);
>+		priv->fpe_wq = NULL;
>+	}
> 
> 	netdev_info(priv->dev, "FPE workqueue stop");
> }
>-- 
>2.34.1
>
>