Re: [PATCH] cxl/test: reject wrapped GET_LOG offsets

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Integer underflow in `mock_transfer_fw()` bypasses bounds check leading to kernel crash
- [High] Unpatched integer overflows in `mock_get_lsa()` and `mock_set_lsa()` bounds checks
- [High] NULL pointer dereferences in mock CXL mailbox handlers due to unvalidated payload sizes
--

commit 5bf785b4ab5b40166bbb1f3af01c1403ead02ea9
Author: Samuel Moelius <sam.moelius@trailofbits.com>

cxl/test: reject wrapped GET_LOG offsets

The CXL mock mailbox GET_LOG handler validates the requested CEL slice
with an offset + length check that is vulnerable to integer wrapping.
This commit fixes the potential wrap by validating the offset first.

> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 271c7ad8cc32c..5dc9601a2a7e4 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -584,7 +584,7 @@ static int mock_get_log(struct cxl_memdev_state *mds, struct cxl_mbox_cmd *cmd)
>  		return -EINVAL;
>  	if (length > cxl_mbox->payload_size)
>  		return -EINVAL;
> -	if (offset + length > sizeof(mock_cel))
> +	if (offset > sizeof(mock_cel) || length > sizeof(mock_cel) - offset)
>  		return -EINVAL;
>  	if (!uuid_equal(&gl->uuid, &uuid))
>  		return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but earlier in mock_get_log(),
are we dereferencing gl->offset before validating cmd->size_in?

static int mock_get_log(struct cxl_memdev_state *mds, struct cxl_mbox_cmd *cmd)
{
	struct cxl_mbox_get_log *gl = cmd->payload_in;
	u32 offset = le32_to_cpu(gl->offset);
	...
	if (cmd->size_in < sizeof(*gl))
		return -EINVAL;

If cmd->size_in is 0, could cmd->payload_in be NULL, leading to a NULL pointer
dereference here? This same pattern appears to exist in mock_clear_event()
and mock_get_test_feature() as well.

[Severity: High]
This is a pre-existing issue, but since you are fixing an integer overflow
here, should we also fix identical unpatched bounds checks in mock_get_lsa()
and mock_set_lsa()?

Both of those functions appear to use the same vulnerable check:

	if (offset + length > LSA_SIZE)

If a very large offset and a small length are provided, couldn't this wrap
around, bypassing the bounds check and allowing out-of-bounds memory
accesses?

[Severity: Critical]
This isn't a bug introduced by this patch, but does mock_transfer_fw() have
a similar bounds checking regression that could cause a crash?

In mock_transfer_fw(), the length is calculated without first validating
cmd->size_in against sizeof(*transfer):

	length = cmd->size_in - sizeof(*transfer);
	if (offset + length > FW_SIZE)

If cmd->size_in is smaller than sizeof(*transfer), wouldn't length underflow
to a massive value? Combined with a specifically chosen offset, could this
wrap around the addition, bypass the bounds check, and result in an
out-of-bounds memcpy()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605142036.2062347-1-sam.moelius@trailofbits.com?part=1