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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The newly introduced feature transfer size validation incorrectly rejects valid reads/writes, causing an off-by-one bounds check error.

Pre-existing issues:
- [High] Missing bounds check on user-provided `out_len` and `count` leads to arbitrary heap buffer overflow and potential `ZERO_SIZE_PTR` dereference in `cxlctl_get_feature` and `cxlctl_set_feature`.
- [High] Missing bounds check on user-supplied `in_len` and `op_size` leads to out-of-bounds heap reads and potential kernel crashes in `cxlctl_set_feature`.
- [High] Hardware-induced heap buffer overflow in `cxl_get_feature()` loop due to incorrect static `size_out` tracking.
- [Medium] Internal mailbox command failures mask their error codes and return SUCCESS to the `fwctl` user.
--

--- Patch [1]: [PATCH v3 1/5] cxl/features: Reject feature offset that overflows 16-bit field ---
commit c16baccf7c99ea7335aa40b18d19b96e88ef46aa
Author: Richard Cheng <icheng@nvidia.com>

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

cxl_get_feature() and cxl_set_feature() build each mailbox command's
offset from the starting offset plus the amount of data already
transferred, then store it in a 16-bit field. An user-controlled fwctl
offset and transfer size can exceed the feature extent, allowing a later
offset to be truncated by cpu_to_le16() and target the wrong feature
data.

> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 85185af46b72d..aaa408445fd40 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -235,7 +235,10 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>  		*return_code = CXL_MBOX_CMD_RC_INPUT;
>  
>  	if (!feat_out || !feat_out_size)
> -		return 0;
> +		return -EINVAL;
> +
> +	if (feat_out_size > U16_MAX - offset)
> +		return -EINVAL;

[Severity: Medium]
Does this logic contain an off-by-one error?

If a user requests exactly 65536 bytes at offset 0, feat_out_size would be
65536. Since U16_MAX is 65535, 65536 > 65535 - 0 evaluates to true, which
incorrectly rejects a valid read.

[ ... ]

> @@ -258,7 +261,7 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>  		if (rc < 0 || !mbox_cmd.size_out) {
>  			if (return_code)
>  				*return_code = mbox_cmd.return_code;

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
hardware-induced heap buffer overflow?

Looking at the loop just before this check:

    mbox_cmd = (struct cxl_mbox_cmd) {
        ...
        .size_out = size_out,
        .payload_out = feat_out + data_rcvd_size,
        .min_out = data_to_rd_size,
    };
    rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);

The command struct tells the mailbox transport that it has size_out bytes of
space remaining in payload_out, even though the actual remaining space requested
for this iteration is data_to_rd_size. If a buggy or malicious hardware device
ignores min_out and returns up to size_out bytes, could the transport blindly
overwrite memory past the end of feat_out?

[Severity: Medium]
This is also a pre-existing issue, but does this mask internal mailbox command
failures?

When cxl_internal_send_cmd() fails before getting a response (e.g., due to a
timeout or busy state), mbox_cmd.return_code retains its 0-initialized value.
Does setting *return_code to 0 end up propagating a SUCCESS response to the
fwctl user, even though the command failed?

> -			return 0;
> +			return rc < 0 ? rc : -EIO;
>  		}
>  		data_rcvd_size += mbox_cmd.size_out;
>  	} while (data_rcvd_size < feat_out_size);

[ ... ]

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

[Severity: Medium]
Does this have the same off-by-one bounds check error as cxl_get_feature() when
writing a full 64KB extent?

[ ... ]

> @@ -476,16 +483,17 @@ static void *cxlctl_get_feature(struct cxl_features_state *cxlfs,
>  	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);

[Severity: High]
This isn't a bug introduced by this patch, but does this lack a bounds check
against count?

The rpc_out buffer is allocated just above using the user-provided out_size:

    struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
        kvzalloc(out_size, GFP_KERNEL);

However, cxl_get_feature() will write up to count bytes into rpc_out->payload.
If a user calls the FWCTL_RPC ioctl with a small out_len but a large count,
could this lead to an arbitrary heap buffer overflow?

Additionally, if out_size is 0, kvzalloc() returns ZERO_SIZE_PTR. Would the
later assignment of rpc_out->size = 0 or data_size trigger a kernel panic?

Similar missing bounds checks on user-supplied in_len and op_size appear to
exist in cxlctl_set_feature() and cxlctl_fw_rpc(), which could lead to
out-of-bounds heap reads or ZERO_SIZE_PTR dereferences.

> +	data_size = cxl_get_feature(cxl_mbox, &feat_in->uuid,
> +				    feat_in->selection, rpc_out->payload,
> +				    count, offset, &return_code);
>  	*out_len = sizeof(struct fwctl_rpc_cxl_out);
> -	if (!out_size) {
> +	if (data_size <= 0) {
>  		rpc_out->size = 0;
>  		rpc_out->retval = return_code;
>  		return no_free_ptr(rpc_out);
>  	}

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