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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Integer overflow in payload bounds checking allows massive out-of-bounds memory reads and writes in `mock_get_log`, `mock_get_lsa`, and `mock_set_lsa`.
- [Critical] Feature handlers blindly write to `cmd->payload_out` based on parameters derived from the input payload without verifying that the destination buffer is sufficiently sized.
- [High] The `cmd->size_in == 0` checks added in `mock_clear_event` and `mock_get_test_feature` are insufficient to prevent out-of-bounds reads for payloads smaller than the expected structure size.
- [High] The patch misses several mailbox handlers that dereference `cmd->payload_in` without checking its size, and the fix in `mock_get_test_feature` is completely bypassed by its caller.
- [High] `cxl_mock_mbox_send` does not serialize concurrent mailbox commands, leading to widespread race conditions across the mock device state.
--

commit 26d56860aa1c2848111addf4d017916d442515a1
Author: Dave Jiang <dave.jiang@intel.com>

cxl/test: Verify cmd->size_in before accessing payload

Adds 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..5d812b10fbc87 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -312,12 +312,17 @@ static int mock_clear_event(struct device *dev, struct cxl_mbox_cmd *cmd)
>  {
> -	struct cxl_mbox_clear_event_payload *pl = cmd->payload_in;
> +	struct cxl_mbox_clear_event_payload *pl;
>  	struct mock_event_log *log;
> -	u8 log_type = pl->event_log;
> +	u8 log_type;
>  	u16 handle;
>  	int nr;
>  
> +	if (cmd->size_in == 0)
> +		return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but is the size_in == 0 check sufficient?

If userspace provides a payload with cmd->size_in == 1, the check passes.
The handler then casts the truncated buffer to struct
cxl_mbox_clear_event_payload and reads subsequent fields out-of-bounds.

Should this check verify that cmd->size_in >= sizeof(*pl)?

> +	pl = cmd->payload_in;
> +	log_type = pl->event_log;

[ ... ]

> @@ -574,14 +579,19 @@ static int mock_get_log(struct cxl_memdev_state *mds, struct cxl_mbox_cmd *cmd)
>  {
>  	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
> -	struct cxl_mbox_get_log *gl = cmd->payload_in;
> -	u32 offset = le32_to_cpu(gl->offset);
> -	u32 length = le32_to_cpu(gl->length);
>  	uuid_t uuid = DEFINE_CXL_CEL_UUID;
> +	struct cxl_mbox_get_log *gl;
>  	void *data = &mock_cel;
> +	u32 offset;
> +	u32 length;
>  
>  	if (cmd->size_in < sizeof(*gl))
>  		return -EINVAL;
> +
> +	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: Critical]
This isn't a bug introduced by this patch, but does the offset + length
computation overflow 32-bit boundaries here?

Because offset and length are 32-bit unsigned integers derived from user
payloads, a very large offset and a small length could overflow their sum
to a small positive value, passing the size check. The subsequent memcpy
would then allow a massive out-of-bounds read or write up to 4GB away.

Similar integer overflows appear to exist in mock_get_lsa() and
mock_set_lsa().

>  		return -EINVAL;

[ ... ]

> @@ -1415,11 +1429,18 @@ static int mock_get_test_feature(struct cxl_mockmem_data *mdata,
>  				 struct cxl_mbox_cmd *cmd)
>  {
>  	struct vendor_test_feat *output = cmd->payload_out;
> -	struct cxl_mbox_get_feat_in *input = cmd->payload_in;
> -	u16 offset = le16_to_cpu(input->offset);
> -	u16 count = le16_to_cpu(input->count);
> +	struct cxl_mbox_get_feat_in *input;
> +	u16 offset;
> +	u16 count;
>  	u8 *ptr;
>  
> +	if (cmd->size_in == 0)
> +		return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but does checking cmd->size_in == 0 fully
prevent out-of-bounds reads?

If the payload is smaller than struct cxl_mbox_get_feat_in, the check
passes but the cast causes an out-of-bounds read when accessing fields
like input->count.

Also, the caller mock_get_feature() seems to unconditionally evaluate
uuid_equal(&input->uuid, ...) prior to calling mock_get_test_feature():

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))
	...
}

Because input is cmd->payload_in, this leads to a ZERO_SIZE_PTR
dereference before the newly added size_in == 0 check is reached.

Other mailbox handlers like mock_activate_fw() also appear to lack size_in
verification.

> +	input = cmd->payload_in;
> +	offset = le16_to_cpu(input->offset);
> +	count = le16_to_cpu(input->count);
> +
>  	if (offset > sizeof(*output)) {
>  		cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
>  		return -EINVAL;
>  	}
>  
>  	if (offset + count > sizeof(*output)) {
>  		cmd->return_code = CXL_MBOX_CMD_RC_INPUT;
>  		return -EINVAL;
>  	}
>  
>  	ptr = (u8 *)&mdata->test_feat + offset;
>  	memcpy((u8 *)output + offset, ptr, count);

[Severity: Critical]
This isn't a bug introduced by this patch, but does this code perform a
memcpy into cmd->payload_out without validating that
offset + count <= cmd->size_out?

If a user provides an undersized size_out but requests a larger read via
input->count, the memcpy will write past the end of the dynamically
allocated buffer. The same pattern seems to exist in
mock_get_supported_features() when writing to out->ents.

>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does cxl_mock_mbox_send() fail to
serialize concurrent mailbox commands?

Looking at cxl_mock_mbox_send():

tools/testing/cxl/test/mem.c:cxl_mock_mbox_send() {
	struct device *dev = cxl_mbox->host;
	...
}

It dispatches mailbox commands without acquiring cxl_mbox->mbox_mutex,
which violates the subsystem's synchronization model. Concurrent IOCTLs
from userspace can execute mock handlers simultaneously, causing data
races and corruptions on shared mock structures.

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