Re: [PATCH] cxl/features: bound fwctl command payload to the input buffer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] User-controlled output buffer allocation size `out_len` is used without validation, leading to severe heap out-of-bounds writes or OOPS in `cxlctl_get_feature` and `cxlctl_set_feature`.
--

commit 0b6168cb6d2ecfaa5cb9fc6b814d7c56cade18ef
Author: Zhenhao Wan <whi4ed0g@gmail.com>

cxl/features: bound fwctl command payload to the input buffer

This commit addresses out-of-bounds reads in CXL feature commands by ensuring
the input buffer size is sufficiently large to hold the fixed header and
payload. It rejects requests that do not fit within the copied-in buffer at a
single dispatch point.

> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -649,7 +649,13 @@ static void *cxlctl_fw_rpc(struct fwctl_uctx *uctx, enum fwctl_rpc_scope scope,
>  	struct cxl_memdev *cxlmd = fwctl_to_memdev(fwctl_dev);
>  	struct cxl_features_state *cxlfs = to_cxlfs(cxlmd->cxlds);
>  	const struct fwctl_rpc_cxl *rpc_in = in;
> -	u16 opcode = rpc_in->opcode;
> +	u16 opcode;
> +
> +	if (in_len < sizeof(rpc_in->hdr) ||
> +	    rpc_in->op_size > in_len - sizeof(rpc_in->hdr))
> +		return ERR_PTR(-EINVAL);

[Severity: Critical]
This is a pre-existing issue, but since we are validating in_len here,
should we also validate *out_len?

In cxlctl_get_feature(), the user-specified *out_len determines the
allocation size but is used without verifying it can hold the structure
header and payload:

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);
    *out_len = sizeof(struct fwctl_rpc_cxl_out);
    if (!out_size) {
        rpc_out->size = 0;
    ...
}

If *out_len is 0, kvzalloc(0) returns ZERO_SIZE_PTR, which could lead
to an oops when writing to rpc_out->size. If *out_len is smaller than
sizeof(struct fwctl_rpc_cxl_out) + count, could this result in a heap
out-of-bounds write?

A similar pattern exists in cxlctl_set_feature(), where the fixed
header fields are unconditionally written regardless of the allocation
size:

drivers/cxl/core/features.c:cxlctl_set_feature() {
    ...
    out_size = *out_len;

    struct fwctl_rpc_cxl_out *rpc_out __free(kvfree) =
        kvzalloc(out_size, GFP_KERNEL);
    if (!rpc_out)
        return ERR_PTR(-ENOMEM);

    rpc_out->size = 0;
    ...
}

> +
> +	opcode = rpc_in->opcode;
>  
>  	if (!cxlctl_validate_hw_command(cxlfs, rpc_in, scope, opcode))
>  		return ERR_PTR(-EINVAL);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620-cxl-fwctl-oob-v1-1-5758e34d784a@gmail.com?part=1