[PATCH] mfd: qnap-mcu: clear the reply state when a command times out

qnap_mcu_exec() puts the address of an on-stack buffer into the shared
reply state before sending a command:

	unsigned char rx[QNAP_MCU_RX_BUFFER_SIZE];
	...
	reply->data = rx;
	reply->length = length;
	reply->received = 0;

qnap_mcu_receive_buf() clears reply->length once a full reply or a
recognised error reply has arrived, and refuses to touch anything while
it is zero:

	if (!reply->length) {
		dev_warn(dev, "Received %zu bytes, we were not waiting for\n", size);
		return size;
	}

	while (src < end) {
		reply->data[reply->received] = *src++;

The timeout path in qnap_mcu_exec() returns without clearing any of it.
reply->data then keeps pointing into a stack frame that is gone, and
because reply->length is still set the guard in the receive callback
does not fire, so a reply that turns up after the timeout is written
over whatever is using that stack now. bus_lock does not help: the write
comes from the serdev receive callback, not from another caller of
qnap_mcu_exec().

The write stays within QNAP_MCU_RX_BUFFER_SIZE bytes, since the entry
check rejects longer replies and the loop stops at reply->length, so
this is a bounded write to a dead frame rather than an unbounded one.

Clear the length and the pointer on the timeout path, so a late reply is
dropped by the check that is already there.

Fixes: 998f70d1806b ("mfd: Add base driver for qnap-mcu devices")
Cc: [email protected]
Signed-off-by: Ali Ahmet Memis <[email protected]>
---
Found by reading the driver, not from a reported failure; I have no QNAP
NAS to test on. What I checked in the tree: reply->length is only set
back to zero in qnap_mcu_receive_buf(), at the two points where a reply
is complete, and reply->data, reply->length and reply->received are only
set again at the top of the next qnap_mcu_exec(). So between a timeout
returning and the next command there is a window where the state still
describes a frame that has been left.

This is the minimal version. It does not close the window completely:
qnap_mcu_receive_buf() runs without bus_lock, so a reply can still
arrive between the check and the two stores. Closing that properly means
either stopping serdev receive around the timeout or protecting the
reply state with a lock the receive callback can also take, and which of
those fits the driver is your call rather than mine. I went with the
small fix because it removes the stale pointer without changing how the
driver is serialized.

 drivers/mfd/qnap-mcu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c
index 8de974ddac3e..f810cb4ff2ab 100644
--- a/drivers/mfd/qnap-mcu.c
+++ b/drivers/mfd/qnap-mcu.c
@@ -239,6 +239,8 @@ int qnap_mcu_exec(struct qnap_mcu *mcu,
 
 	if (!wait_for_completion_timeout(&reply->done, msecs_to_jiffies(QNAP_MCU_TIMEOUT_MS))) {
 		dev_err(&mcu->serdev->dev, "Command timeout\n");
+		reply->length = 0;
+		reply->data = NULL;
 		return -ETIMEDOUT;
 	}
 

base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952
-- 
2.55.0