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

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.

> 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..."?

> diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c
> index 8de974ddac3e7..f810cb4ff2ab7 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;

[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?

>  		return -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?

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