Re: [PATCH 1/3] cxl/features: Reject feature offset that overflows 16-bit field

[sashiko-bot@kernel.org]

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

New issues:
- [High] The bounds check `offset + feat_data_size > U16_MAX` is vulnerable to a 32-bit integer wrap-around on 32-bit architectures, completely bypassing the validation.

Pre-existing issues:
- [High] The CXL fwctl RPC handlers fail to validate the user-supplied payload size (`rpc_in->op_size`) and structure boundaries against the actual buffer length (`in_len`), allowing out-of-bounds kernel memory reads and ZERO_SIZE_PTR dereferences.
- [High] `cxlctl_get_feature` lacks validation ensuring the output buffer capacity matches the user-requested read count, leading to out-of-bounds kernel memory writes.
- [Medium] In `cxl_get_feature`, the output buffer capacity parameter `mbox_cmd.size_out` is not scaled down for the final iteration, allowing a misbehaving CXL device to overflow the destination buffer.
--

commit 5ab5a6aebf166b549e1dc9a1b5568aa38e4c661e
Author: Richard Cheng <icheng@nvidia.com>

cxl/features: Reject feature offset that overflows 16-bit field

cxl_get_feature() and cxl_set_feature() build the mailbox command's
offset as cpu_to_le16(offset + data_rcvd_size/data_sent_size), but never
check the sum fits in the 16-bit field. Via fwctl, a user-supplied
offset plus count/op_size summing over 65535 silently wraps, steering
the device to the wrong feature offset.

> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 85185af46b72d..db5964ea184f1 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -237,6 +237,9 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>  	if (!feat_out || !feat_out_size)
>  		return 0;
>  
> +	if (offset + feat_out_size > U16_MAX)
> +		return 0;
> +
>  	size_out = min(feat_out_size, cxl_mbox->payload_size);

[Severity: Medium]
This is a pre-existing issue, but in the do...while loop below this code, 
mbox_cmd.size_out is assigned this constant size_out value:

    mbox_cmd = (struct cxl_mbox_cmd) {
        .opcode = CXL_MBOX_OP_GET_FEATURE,
        .size_in = sizeof(pi),
        .payload_in = &pi,
        .size_out = size_out,
        .payload_out = feat_out + data_rcvd_size,
        .min_out = data_to_rd_size,
    };

Is it possible for a misbehaving hardware device to return more data than 
requested in the final iteration? Since the remaining buffer space 
(data_to_rd_size) can be smaller than size_out, could this overrun the 
feat_out buffer by up to (size_out - data_to_rd_size) bytes?

[ ... ]
> @@ -287,6 +290,9 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox,
>  	if (return_code)
>  		*return_code = CXL_MBOX_CMD_RC_INPUT;
>  
> +	if (offset + feat_data_size > U16_MAX)
> +		return -EINVAL;
> +

[Severity: High]
On 32-bit architectures, feat_data_size (a size_t) is a 32-bit unsigned
integer. If a caller provides a large value like U32_MAX - 32 alongside
an offset of 65535, couldn't offset + feat_data_size wrap around modulo 2^32?

This wrap-around could bypass the U16_MAX check, potentially allowing an
out-of-bounds kernel memory read in the loop below. Should this check use
check_add_overflow() or otherwise explicitly guard against wrap-around?

>  	struct cxl_mbox_set_feat_in *pi __free(kfree) =
>  			kzalloc(cxl_mbox->payload_size, GFP_KERNEL);
>  	if (!pi)

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking at how fwctl
handles the get feature RPC in cxlctl_get_feature():

drivers/cxl/core/features.c:cxlctl_get_feature() {
	...
	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
		kvzalloc(out_size, GFP_KERNEL);
	if (!rpc_out)
		return ERR_PTR(-ENOMEM);

	out_size = cxl_get_feature(cxl_mbox, &feat_in->uuid,
				   feat_in->selection, rpc_out->payload,
				   count, offset, &return_code);
	...
}

The output buffer rpc_out is allocated exactly to the user-controlled length
(out_size/out_len), without verifying it can hold the requested payload count.
If a user supplies out_len = 8 (just the header) but asks the device to
read 65535 bytes via feat_in->count, doesn't cxl_get_feature() write the
hardware data past the end of the 8-byte allocation?


[Severity: High]
This is a pre-existing issue, but the CXL fwctl RPC handlers appear to use
the user-supplied length fields without validating them against the actual
allocated buffer size:

drivers/cxl/core/features.c:cxlctl_fw_rpc() {
	...
	const struct fwctl_rpc_cxl *rpc_in = in;
	u16 opcode = rpc_in->opcode;
	...
}

If the incoming buffer in_len is 0, in could be ZERO_SIZE_PTR, which would
cause a panic when dereferencing rpc_in->opcode.

Furthermore, in cxlctl_set_feature():

drivers/cxl/core/features.c:cxlctl_set_feature() {
	...
	data_size = rpc_in->op_size - sizeof(feat_in->hdr);
	rc = cxl_set_feature(cxl_mbox, &feat_in->uuid,
			     feat_in->version, feat_in->feat_data,
			     data_size, flags, offset, &return_code);
	...
}

If the input buffer is smaller than the structure size, but rpc_in->op_size
is spoofed to a large valid size, this derives data_size from the unvalidated
op_size rather than the verified in_len. Could this cause cxl_set_feature()
to copy out-of-bounds kernel memory to the hardware mailbox?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630074657.43077-1-icheng@nvidia.com?part=1