Re: [PATCH v2] mfd: qnap-mcu: keep the reply buffer alive past a command timeout

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Low] The commit message description is not capitalized, violating MFD subsystem formatting rules.

Pre-existing issues:
- [Critical] A data race between `qnap_mcu_exec()` and `qnap_mcu_receive_buf()` on `reply->length` and `reply->received` bypasses the loop exit condition, leading to a severe heap buffer overflow.
- [High] Late UART replies from timed-out or failed commands can be written into the active buffer of the next command, causing data cross-talk and spurious early completion.
- [High] A data race can cause `qnap_mcu_verify_checksum()` to be called with `reply->received == 0`, leading to an integer underflow and a deterministic kernel panic.
--

--- Patch [1]: [PATCH v2] mfd: qnap-mcu: keep the reply buffer alive past a command timeout ---
commit 876c0f2fd3f8e4b677ad50b82d00f3dcfe4a3283
Author: Ali Ahmet Memis <[email protected]>

mfd: qnap-mcu: keep the reply buffer alive past a command timeout

This patch moves the receive buffer from the stack in qnap_mcu_exec() to
the qnap_mcu structure. This prevents stack corruption when late replies
arrive after a command has timed out.

Link: https://lore.kernel.org/all/[email protected]/

> mfd: qnap-mcu: keep the reply buffer alive past a command timeout

[Severity: Low]
Is there a capitalization typo in the subject line? Should "keep" be "Keep"?

> diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c
> index 8de974ddac3e7..93a3dd93404dd 100644
> --- a/drivers/mfd/qnap-mcu.c
> +++ b/drivers/mfd/qnap-mcu.c

[ ... ]

> @@ -214,19 +216,18 @@ int qnap_mcu_exec(struct qnap_mcu *mcu,
>  		  const u8 *cmd_data, size_t cmd_data_size,
>  		  u8 *reply_data, size_t reply_data_size)
>  {
> -	unsigned char rx[QNAP_MCU_RX_BUFFER_SIZE];
>  	size_t length = reply_data_size + QNAP_MCU_CHECKSUM_SIZE;
>  	struct qnap_mcu_reply *reply = &mcu->reply;
>  	int ret = 0;
>  
> -	if (length > sizeof(rx)) {
> +	if (length > sizeof(mcu->rx)) {
>  		dev_err(&mcu->serdev->dev, "expected data too big for receive buffer");
>  		return -EINVAL;
>  	}
>  
>  	guard(mutex)(&mcu->bus_lock);
>  
> -	reply->data = rx;
> +	reply->data = mcu->rx;
>  	reply->length = length;
>  	reply->received = 0;

[Severity: Critical]
This is a pre-existing issue, but can this reset race with an active receive
loop and cause a heap buffer overflow?

If the receive thread is running in qnap_mcu_receive_buf() and we reset
reply->received to 0 here without synchronization, the exact equality check
in the receive thread could be bypassed:

drivers/mfd/qnap-mcu.c:qnap_mcu_receive_buf()
	reply->received++;
	if (reply->received == reply->length) {
		...

If the equality check is bypassed, the receive loop could write unbounded
bytes into reply->data (which now points to mcu->rx on the heap), causing
a heap buffer overflow.

>  	reinit_completion(&reply->done);

[Severity: High]
This isn't a bug introduced by this patch, but can resetting the completion
here allow late replies to corrupt the new command?

If a previous command times out, its receive thread is still active.

If we just reset the completion and start a new command, could a concurrently
arriving late response from the previous command be written into mcu->rx by
qnap_mcu_receive_buf()?

Since the old response is well-formed, it could pass the simple XOR checksum,
triggering a spurious complete() and causing the new command to instantly
return with stale data.

>  
>  	ret = qnap_mcu_write(mcu, cmd_data, cmd_data_size);
>  	if (ret < 0)
>  		return ret;
>  
>  	serdev_device_wait_until_sent(mcu->serdev, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS));
>  
>  	if (!wait_for_completion_timeout(&reply->done, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS))) {
>  		dev_err(&mcu->serdev->dev, "Command timeout\n");
>  		return -ETIMEDOUT;
>  	}
>  
> -	if (!qnap_mcu_verify_checksum(rx, reply->received)) {
> +	if (!qnap_mcu_verify_checksum(mcu->rx, reply->received)) {

[Severity: High]
This is a pre-existing issue, but could this cause an integer underflow and a
kernel panic if reply->received is 0?

If qnap_mcu_receive_buf() evaluates reply->received == reply->length and
zeroes reply->length, but is preempted before calling complete(), the
original command can time out. 

A new command could then call qnap_mcu_exec(), setting reply->received to 0.

If the preempted receive thread then resumes and calls complete(), the new
command will wake up and call qnap_mcu_verify_checksum() with
reply->received == 0:

drivers/mfd/qnap-mcu.c:qnap_mcu_verify_checksum()
	u8 crc = qnap_mcu_csum(buf, size - QNAP_MCU_CHECKSUM_SIZE);

If size is 0, this underflows to the maximum size value, and qnap_mcu_csum()
will loop reading sequentially out of bounds until it hits an unmapped page.

>  		dev_err(&mcu->serdev->dev, "Invalid Checksum received from controller\n");
>  		return -EPROTO;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1