Re: [PATCH] cxl/memdev: Fix firmware upload exact-fit handling

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

Pre-existing issues:
- [High] Unvalidated firmware slot count (`num_slots`) from the device leads to a divide-by-zero kernel panic during firmware upload.
- [High] The firmware transfer INITIATE action does not set the target slot, leaving it as 0 which is invalid and breaks multi-part uploads.
--

--- Patch [1]: [PATCH] cxl/memdev: Fix firmware upload exact-fit handling ---
commit bd3fd9a0f6bb2e86d5487d6d79f980a791c0b33e
Author: Guzebing <[email protected]>

cxl/memdev: Fix firmware upload exact-fit handling

cxl_fw_prepare() classifies a firmware image as a one-shot transfer
only when its Transfer FW input payload is smaller than the mailbox
payload size. An image that exactly fills the payload is therefore
treated as a multi-part transfer.

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 2e457b1ebc7df..b3419df586b9c 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -988,7 +988,7 @@ static enum fw_upload_err cxl_fw_prepare(struct fw_upload *fwl, const u8 *data,
>  	if (!size)
>  		return FW_UPLOAD_ERR_INVALID_SIZE;
>  
> -	mds->fw.oneshot = struct_size(transfer, data, size) <
> +	mds->fw.oneshot = struct_size(transfer, data, size) <=
>  			    cxl_mbox->payload_size;
>  
>  	if (cxl_mem_get_fw_info(mds))

[Severity: High]
This isn't a bug introduced by this patch, but unvalidated firmware slot count
from the device can lead to a divide-by-zero kernel panic during firmware
upload.

If a malicious or buggy device returns 0 for info.num_slots here:

drivers/cxl/core/memdev.c:cxl_mem_get_fw_info()
    ...
    mds->fw.num_slots = info.num_slots;
    ...

Later, when a user initiates a firmware upload, cxl_fw_write() will trigger an
unhandled divide-by-zero exception:

drivers/cxl/core/memdev.c:cxl_fw_write()
    ...
    mds->fw.next_slot = (mds->fw.cur_slot % mds->fw.num_slots) + 1;
    ...

Should we validate that info.num_slots is non-zero before assigning it?


[Severity: High]
This is a pre-existing issue, but the firmware transfer INITIATE action does
not set the target slot, leaving it as 0 which is invalid and breaks
multi-part uploads.

For multi-part uploads, transfer is allocated with kzalloc and transfer->slot
is explicitly left unset in the INITIATE branch:

drivers/cxl/core/memdev.c:cxl_fw_write()
    ...
    transfer = kzalloc(size_in, GFP_KERNEL);
    ...
    if (mds->fw.oneshot) {
        ...
    } else {
        if (offset == 0) {
            transfer->action = CXL_FW_TRANSFER_ACTION_INITIATE;
        } else if (remaining == 0) {
            ...
    ...

Since CXL firmware slots are 1-indexed, and the CXL specification requires a
valid target slot for Initiate actions, the driver sends slot 0 and
permanently fails.

Does transfer->slot need to be set to mds->fw.next_slot in the INITIATE action
block as well?

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