[PATCH v2] cxl/features: Reject Get Feature count larger than the output buffer

[PATCH v2] cxl/features: Reject Get Feature count larger than the output buffer

[Richard Cheng]

cxlctl_get_feature() sizes its output buffer from the user's
fwctl_rpc.out_len, but the device is told to write
cxl_mbox_get_feat_in.count bytes into rpc_out->payload, which is a
separate user-controlled value. Nothing bounds count against out_len, so
a small out_len with a large count overflows the kvzalloc()'d buffer.
A heap OOB write reachable from FWCTL_RPC.

Reject requests where count exceeds the available payload room, before
allocating.

Fixes: 5908f3ed6dc2 ("cxl: Add support to handle user feature commands for get feature")
Reviewed-by: Kai-Heng Feng <kaihengf@nvidia.com>
Reviewed-by: Koba Ko <kobak@nvidia.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>
Signed-off-by: Richard Cheng <icheng@nvidia.com>
---
Changelog:

v1 -> v2:
    - Drop the reproducer and trim the KASAN splat in the commit message
    - Sent the reproducer as a regression test in ndctl separately.

Best regards,
Richard Cheng
---
 drivers/cxl/core/features.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
index 85185af46b72..9c714ee42a41 100644
--- a/drivers/cxl/core/features.c
+++ b/drivers/cxl/core/features.c
@@ -471,6 +471,10 @@ static void *cxlctl_get_feature(struct cxl_features_state *cxlfs,
 	if (!count)
 		return ERR_PTR(-EINVAL);
 
+	if (out_size < offsetof(struct fwctl_rpc_cxl_out, payload) ||
+	    count > out_size - offsetof(struct fwctl_rpc_cxl_out, payload))
+		return ERR_PTR(-EINVAL);
+
 	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
 		kvzalloc(out_size, GFP_KERNEL);
 	if (!rpc_out)

base-commit: ef0c9f75a19532d7675384708fc8621e10850104
-- 
2.43.0

Re: [PATCH v2] cxl/features: Reject Get Feature count larger than the output buffer

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing `out_len` bounds check in `cxlctl_set_feature()` leads to heap out-of-bounds write or `ZERO_SIZE_PTR` dereference panic.
- [High] Missing `in_len` bounds validation across all CXL fwctl handlers allows triggering a kernel panic or reading arbitrary kernel heap memory into CXL devices.
- [High] A malicious or malfunctioning CXL device can trigger a heap buffer overflow in `cxl_get_feature` by returning more data than requested in subsequent iterations.
--

commit 2b1a2543e64d377118a202ab8f46bc2a74fccabb
Author: Richard Cheng <icheng@nvidia.com>

cxl/features: Reject Get Feature count larger than the output buffer

cxlctl_get_feature() sizes its output buffer from the user's
fwctl_rpc.out_len, but the device is told to write
cxl_mbox_get_feat_in.count bytes into rpc_out->payload, which is a
separate user-controlled value. Nothing bounds count against out_len, so
a small out_len with a large count overflows the kvzalloc()'d buffer.

> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 85185af46b72d..9c714ee42a411 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -471,6 +471,10 @@ static void *cxlctl_get_feature(struct cxl_features_state *cxlfs,
>  	if (!count)
>  		return ERR_PTR(-EINVAL);
>  
> +	if (out_size < offsetof(struct fwctl_rpc_cxl_out, payload) ||
> +	    count > out_size - offsetof(struct fwctl_rpc_cxl_out, payload))
> +		return ERR_PTR(-EINVAL);
> +
>  	struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
>  		kvzalloc(out_size, GFP_KERNEL);
>  	if (!rpc_out)
>  		return ERR_PTR(-ENOMEM);

[Severity: High]
This isn't a bug introduced by this patch, but it looks like
cxlctl_set_feature() might have a similar missing out_len bounds check.

If a user specifies a very small out_len (e.g., 0) for the fwctl rpc ioctl
with cxlctl_set_feature, could this cause a zero size pointer dereference or
a heap out-of-bounds write?

drivers/cxl/core/features.c:cxlctl_set_feature() {
    ...
    struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
        kvzalloc(out_size, GFP_KERNEL);
    ...
    rpc_out->size = 0;
    ...
}

[Severity: High]
This is a pre-existing issue, but does cxlctl_fw_rpc() need to validate
in_len before dereferencing the input pointer?

If in_len is 0, would reading the opcode cause a kernel panic?

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

Additionally, in cxlctl_set_feature(), data_size is derived from op_size
rather than the actual in_len:

drivers/cxl/core/features.c:cxlctl_set_feature() {
    ...
    data_size = rpc_in->op_size - sizeof(feat_in->hdr);
    ...
}

Could this lead to an out-of-bounds read of the kernel heap in
cxl_set_feature() if the user provides a maliciously large op_size?

drivers/cxl/core/features.c:cxl_set_feature() {
    ...
    memcpy(pi->feat_data, feat_data + data_sent_size, data_in_size);
    ...
}

[Severity: High]
This is a pre-existing issue, but can a malfunctioning CXL device trigger
a heap buffer overflow in cxl_get_feature()?

The size_out capacity is calculated once before the loop:

drivers/cxl/core/features.c:cxl_get_feature() {
    ...
    size_out = min(feat_out_size, cxl_mbox->payload_size);
    ...
    while (data_to_rd_size > 0) {
        ...
        mbox_cmd.size_out = size_out;
        ...
    }
}

If feat_out_size is larger than the payload size, and the hardware returns
more data than data_to_rd_size in the final iteration, will this overflow the
feat_out allocation since the remaining space at feat_out + data_rcvd_size
might be smaller than size_out?

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