Re: [PATCH v2] cxl/test: Verify cmd->size_in before accessing payload

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete bounds check in mock_clear_event leads to an out-of-bounds read vulnerability.
- [High] NULL pointer dereferences in mock feature handlers due to bounds checks being placed too late or missing entirely.
- [High] Out-of-bounds write on the heap in mock handlers due to missing or inadequate output bounds validation.
- [High] Integer overflow in mock_get_log bypasses bounds checks and causes a massive out-of-bounds read.
--

commit 95b18e396910f0c346a2bf6b9e8a26169223eef2
Author: Dave Jiang <dave.jiang@intel.com>
cxl/test: Verify cmd->size_in before accessing payload

Add a check for cmd->size_in before accessing the payload. This prevents
potential dereferencing of a null pointer if size_in is zero.

Link: https://lore.kernel.org/linux-cxl/20260605143748.235271F00893@smtp.kernel.org/

> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 271c7ad8cc32c..2e9a5f151e983 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -312,12 +312,17 @@ static int mock_get_event(struct device *dev, struct cxl_mbox_cmd *cmd)
>  
>  static int mock_clear_event(struct device *dev, struct cxl_mbox_cmd *cmd)
>  {
[ ... ]
> +
> +	if (cmd->size_in < sizeof(*pl))
> +		return -EINVAL;
> +
> +	pl = cmd->payload_in;
> +	log_type = pl->event_log;

[Severity: High]
This isn't a bug introduced by this patch, but since we are adding a bounds
check here, does it fully protect against out-of-bounds reads?

Later in this function, there is a loop:

tools/testing/cxl/test/mem.c:mock_clear_event() {
    ...
    for (nr = 0, handle = event_get_clear_handle(log);
         nr < pl->nr_recs;
         nr++, handle++) {
        if (handle != le16_to_cpu(pl->handles[nr])) {
    ...
}

If the user provides a payload where size_in only covers the header (exactly
sizeof(*pl)) but sets nr_recs > 0, won't this loop read past the end of the
cmd->payload_in buffer?

[ ... ]
> @@ -574,14 +579,19 @@ static int mock_gsl(struct cxl_mbox_cmd *cmd)
>  static int mock_get_log(struct cxl_memdev_state *mds, struct cxl_mbox_cmd *cmd)
>  {
[ ... ]
> +
> +	gl = cmd->payload_in;
> +	offset = le32_to_cpu(gl->offset);
> +	length = le32_to_cpu(gl->length);
> +
>  	if (length > cxl_mbox->payload_size)
>  		return -EINVAL;
>  	if (offset + length > sizeof(mock_cel))

[Severity: High]
This isn't a bug introduced by this patch, but is there an integer overflow
in this bounds check?

Both offset and length are u32 variables controlled by the user. If offset is
very large and length is small, could offset + length wrap around to a small
number and bypass this check?

This might lead to an out-of-bounds read in the subsequent memcpy():

tools/testing/cxl/test/mem.c:mock_get_log() {
    ...
    memcpy(cmd->payload_out, data + offset, length);
    ...
}

[ ... ]
> @@ -1415,11 +1429,18 @@ static int mock_get_test_feature(struct cxl_mockmem_data *mdata,
>  				 struct cxl_mbox_cmd *cmd)
>  {
[ ... ]
> +	if (cmd->size_in < sizeof(*input))
> +		return -EINVAL;
> +
> +	input = cmd->payload_in;
> +	offset = le16_to_cpu(input->offset);
> +	count = le16_to_cpu(input->count);
> +

[Severity: High]
This isn't a bug introduced by this patch, but while this check prevents null
pointer dereferences within mock_get_test_feature(), its caller
mock_get_feature() appears to unconditionally dereference cmd->payload_in
before getting here:

tools/testing/cxl/test/mem.c:mock_get_feature() {
    ...
    struct cxl_mbox_get_feat_in *input = cmd->payload_in;

    if (uuid_equal(&input->uuid, &CXL_VENDOR_FEATURE_TEST))
    ...
}

If userspace sends a command with size_in = 0, will it trigger a panic before
ever reaching this new check? Similar omissions appear to exist in
mock_set_feature() and mock_get_supported_features().

[Severity: High]
This isn't a bug introduced by this patch, but while looking at bounds
validation in these mock handlers, is there an out-of-bounds write on the
heap in other functions?

For example, mock_get_supported_features() writes to cmd->payload_out without
validating that cmd->size_out is large enough:

tools/testing/cxl/test/mem.c:mock_get_supported_features() {
    ...
    out->supported_feats = cpu_to_le16(MAX_CXL_TEST_FEATS);
    cmd->return_code = 0;
    if (count < struct_size(out, ents, MAX_CXL_TEST_FEATS)) {
        out->num_entries = 0;
        return 0;
    }

    out->num_entries = cpu_to_le16(MAX_CXL_TEST_FEATS);
    feat = out->ents;
    fill_feature_vendor_test(feat);
    ...
}

This function checks the requested input count instead of the actual allocated
cmd->size_out before writing 48 bytes to the flexible array out->ents.
Similarly, mock_get_poison() copies dynamically sized poison records to
payload_out without checking if cmd->size_out is large enough.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605181508.3981107-1-dave.jiang@intel.com?part=1