Re: [PATCH net] pds_core: Fix potential invalid stack memory access

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
pds_core: Fix potential invalid stack memory access

This commit attempts to fix a potential stack corruption issue when an admin
queue command times out. It adds a spinlock around the timeout completion
check to prevent concurrent access by the timeout handler and the completion
handler.

> diff --git a/drivers/net/ethernet/amd/pds_core/adminq.c b/drivers/net/ethernet/amd/pds_core/adminq.c
> --- a/drivers/net/ethernet/amd/pds_core/adminq.c
> +++ b/drivers/net/ethernet/amd/pds_core/adminq.c
> @@ -283,9 +283,15 @@ int pdsc_adminq_post(struct pdsc *pdsc,
[ ... ]
>  		remaining = wait_for_completion_timeout(wc, poll_jiffies);
>  		if (remaining)
>  			break;
>  
>  		if (!pdsc_is_fw_running(pdsc)) {
[ ... ]
>  			err = -ENXIO;
>  			break;
>  		}
[ ... ]
>  	} while (time_before(jiffies, time_limit));
>  	time_done = jiffies;
[ ... ]
>  	/* Check the results and clear an un-completed timeout */
> -	if (time_after_eq(time_done, time_limit) && !completion_done(wc)) {
> -		err = -ETIMEDOUT;
> -		complete(wc);
> +	if (time_after_eq(time_done, time_limit)) {
> +		unsigned long irqflags;
> +
> +		spin_lock_irqsave(&pdsc->adminq_lock, irqflags);

Can an ABA race condition occur if the thread is preempted right before
acquiring the adminq_lock?

If the thread times out and is preempted just before spin_lock_irqsave(),
the hardware could complete the command, and a new thread might wrap the
admin queue, reusing the same slot. The new thread would call
reinit_completion(wc), resetting wc->done to 0.

When the preempted thread resumes, completion_done(wc) would evaluate to
false, causing it to erroneously complete the new command prematurely.

> +		if (!completion_done(wc)) {
> +			err = -ETIMEDOUT;
> +			complete(wc);
> +		}
> +		spin_unlock_irqrestore(&pdsc->adminq_lock, irqflags);
>  	}

Does this leave the stack memory corruption possible on the -ENXIO error
path?

If the wait loop breaks early because pdsc_is_fw_running() evaluates to false,
time_done may be less than time_limit. The code would bypass this newly added
locked block and return without calling complete(wc). Could the interrupt
handler later write to the destroyed stack variable comp?

Will successful commands that complete right near the timeout limit
incorrectly return -ETIMEDOUT?

If wait_for_completion_timeout() successfully acquires the completion, it
decrements wc->done to 0 and breaks. If jiffies has advanced past time_limit
due to thread preemption, completion_done(wc) will evaluate to false inside
this block because wc->done is 0.

This sets err to -ETIMEDOUT, which isn't cleared on success, triggering an
unintended device reset down the err_out path.