mfd: qnap-mcu: clear the reply state when a command times out
Subsystems (3)
PATCH 1/1 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.
---
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(+)
drivers/mfd/qnap-mcu.c +2 -0
@@ -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
Signature · 1 line
2.55.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [Critical] Lockless clearance of reply->length and reply->data on the timeout path introduces a TOCTOU race leading to a NULL pointer dereference or unbounded stack overflow.
- [Low] The commit message description is not capitalized.
Pre-existing issues:
- [High] Unhandled error path from qnap_mcu_write fails to clear the reply state, leaving a stale pointer to a destroyed stack frame.
- [High] Dynamic modification of a non-const global mfd_cell array during driver probe violates MFD subsystem rules and introduces a data race.
- [High] Lockless design of qnap_mcu_receive_buf() causes data races against qnap_mcu_exec() initialization, leading to cross-command state corruption on unhandled timeouts.
- [High] Passing complex structure pointers directly through the .data field in the of_device_id match table violates MFD subsystem guidelines. --
--- Patch [1]: [PATCH] mfd: qnap-mcu: clear the reply state when a command times out --- commit 89b6394df945cf63d91370772916c2e3c424866f
Author: Ali Ahmet Memis <[email protected]>
mfd: qnap-mcu: clear the reply state when a command times out
This commit modifies the timeout path in qnap_mcu_exec() to clear the reply length and pointer. The goal is to prevent a late asynchronous reply from being written into a destroyed on-stack buffer.
Quoted 1 line
mfd: qnap-mcu: clear the reply state when a command times out
[Severity: Low]
This isn't a bug introduced by this patch, but the MFD subsystem guidelines require capitalizing the description after the subsystem prefix. Could we update the subject to "mfd: qnap-mcu: Clear the reply state..."?
drivers/mfd/qnap-mcu.c int qnap_mcu_exec(struct qnap_mcu *mcu, · line 243
Quoted 10 lines
drivers/mfd/qnap-mcu.c +2
-0@@ -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 ;
[Severity: Critical]
Does clearing these fields locklessly introduce a race condition with the asynchronous receive callback?
If qnap_mcu_receive_buf() is actively executing:
qnap_mcu_receive_buf()
if (!reply->length)
return size;
while (src < end) {
reply->data[reply->received] = *src++;
...
if (reply->received == reply->length)
...If the compiler reloads reply->data after it is set to NULL, couldn't this cause a NULL pointer dereference?
Additionally, if it caches reply->data but reloads reply->length as 0, the termination condition reply->received == reply->length becomes impossible to satisfy. Could this cause the loop to blindly copy incoming bytes and create an unbounded stack overflow?
drivers/mfd/qnap-mcu.c int qnap_mcu_exec(struct qnap_mcu *mcu, · line 245
Quoted 2 lines
drivers/mfd/qnap-mcu.c +0
-0return -ETIMEDOUT ; }
[Severity: High]
This is a pre-existing issue, but the error path for qnap_mcu_write() in qnap_mcu_exec() appears to leave a stale pointer:
qnap_mcu_exec() {
...
reply->data = rx;
reply->length = length;
...
ret = qnap_mcu_write(mcu, cmd_data, cmd_data_size);
if (ret < 0)
return ret;If qnap_mcu_write() fails, we return without clearing reply->length and reply->data. Could an asynchronous UART receive callback write into the destroyed stack frame here?
[Severity: High]
This is a pre-existing issue, but does the lockless design of qnap_mcu_receive_buf() allow cross-command state corruption?
In qnap_mcu_exec(), we initialize the reply locklessly:
qnap_mcu_exec() {
...
reply->data = rx;
reply->length = length;
reply->received = 0;
...If a previous command timed out and a late reply arrives while these fields are being initialized for the next command, could the late reply bytes corrupt the new command's buffer or incorrectly trigger its completion?
[Severity: High]
This is a pre-existing issue, but does qnap_mcu_probe() dynamically modify a shared global mfd_cell array?
Looking at qnap_mcu_cells:
static struct mfd_cell qnap_mcu_cells[] = {
...
};qnap_mcu_probe() {
...
qnap_mcu_cells[i].platform_data = mcu->variant;
qnap_mcu_cells[i].pdata_size = sizeof(*mcu->variant);
}MFD subsystem rules require mfd_cell arrays to be static and constant. If two QNAP MCU devices are probed concurrently, could they mutate this shared global array simultaneously and cause a data race?
[Severity: High]
This is a pre-existing issue, but is it safe to pass complex structure pointers through the match table data field?
qnap_mcu_dt_ids[] = {
{ .compatible = "qnap,ts133-mcu", .data = &qnap_ts133_mcu },
...
};The MFD subsystem guidelines state that passing complex pointers through match data tables can cause memory safety hazards and initialization races. The guidelines mandate using an enum or integer ID instead. Should this be updated to select the static struct in the probe function?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1