[PATCH] cxl/test: reject wrapped GET_LOG offsets

[PATCH] cxl/test: reject wrapped GET_LOG offsets

[Samuel Moelius]

The CXL mock mailbox GET_LOG handler validates the requested CEL slice
with `offset + length > sizeof(mock_cel)`.  Both fields come from the
userspace CXL_MEM_SEND_COMMAND payload and are 32-bit values, so an
offset near U32_MAX can wrap the addition to a small value and pass the
bounds check.

The wrapped request then uses the original large offset as the source
address for memcpy(), reading far outside the mock CEL array.

Validate the offset first and compare the length against the remaining
CEL size so the check cannot wrap.

Assisted-by: Codex:gpt-5.5-cyber-preview
Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
---
 tools/testing/cxl/test/mem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
index 271c7ad8cc32..5dc9601a2a7e 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;
-- 
2.43.0

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

[sashiko-bot]

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

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

[Dave Jiang]

On 6/5/26 7:20 AM, Samuel Moelius wrote:
> The CXL mock mailbox GET_LOG handler validates the requested CEL slice
> with `offset + length > sizeof(mock_cel)`.  Both fields come from the
> userspace CXL_MEM_SEND_COMMAND payload and are 32-bit values, so an
> offset near U32_MAX can wrap the addition to a small value and pass the
> bounds check.
> 
> The wrapped request then uses the original large offset as the source
> address for memcpy(), reading far outside the mock CEL array.
> 
> Validate the offset first and compare the length against the remaining
> CEL size so the check cannot wrap.
> 
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  tools/testing/cxl/test/mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 271c7ad8cc32..5dc9601a2a7e 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;

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

[Alison Schofield]

On Fri, Jun 05, 2026 at 02:20:31PM +0000, Samuel Moelius wrote:
> The CXL mock mailbox GET_LOG handler validates the requested CEL slice
> with `offset + length > sizeof(mock_cel)`.  Both fields come from the
> userspace CXL_MEM_SEND_COMMAND payload and are 32-bit values, so an
> offset near U32_MAX can wrap the addition to a small value and pass the
> bounds check.
> 
> The wrapped request then uses the original large offset as the source
> address for memcpy(), reading far outside the mock CEL array.
> 
> Validate the offset first and compare the length against the remaining
> CEL size so the check cannot wrap.
> 
> Assisted-by: Codex:gpt-5.5-cyber-preview
> Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>

Hi Samuel,

I'd suggest keeping the commit log focused on the broken property and
how the fix restores it, rather than tracing the individual arithmetic
operations and later accesses, which are already evident from the code.

The GET_LOG handler is intended to reject requests that describe a CEL
range extending beyond the available data. The current validation can
incorrectly accept some malformed requests because of arithmetic
wraparound, and the fix restores that property by validating the
requested range in a way that cannot overflow.

The discussion of the subsequent memcpy() access leaves me wondering
what the observable effect actually is. Does this return bogus CEL
data, trigger KASAN, crash the test module, or something else? If there
is a demonstrated failure, please describe it. Otherwise, I think the
property being restored is the more important aspect to capture in the
commit log.

-- Alison


> ---
>  tools/testing/cxl/test/mem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 271c7ad8cc32..5dc9601a2a7e 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;
> -- 
> 2.43.0
> 

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

[Samuel Moelius]

On Wed, Jun 10, 2026 at 2:01 PM Alison Schofield
<alison.schofield@intel.com> wrote:
>
> On Fri, Jun 05, 2026 at 02:20:31PM +0000, Samuel Moelius wrote:
> > The CXL mock mailbox GET_LOG handler validates the requested CEL slice
> > with `offset + length > sizeof(mock_cel)`.  Both fields come from the
> > userspace CXL_MEM_SEND_COMMAND payload and are 32-bit values, so an
> > offset near U32_MAX can wrap the addition to a small value and pass the
> > bounds check.
> >
> > The wrapped request then uses the original large offset as the source
> > address for memcpy(), reading far outside the mock CEL array.
> >
> > Validate the offset first and compare the length against the remaining
> > CEL size so the check cannot wrap.
> >
> > Assisted-by: Codex:gpt-5.5-cyber-preview
> > Signed-off-by: Samuel Moelius <sam.moelius@trailofbits.com>
>
> Hi Samuel,
>
> I'd suggest keeping the commit log focused on the broken property and
> how the fix restores it, rather than tracing the individual arithmetic
> operations and later accesses, which are already evident from the code.
>
> The GET_LOG handler is intended to reject requests that describe a CEL
> range extending beyond the available data. The current validation can
> incorrectly accept some malformed requests because of arithmetic
> wraparound, and the fix restores that property by validating the
> requested range in a way that cannot overflow.
>
> The discussion of the subsequent memcpy() access leaves me wondering
> what the observable effect actually is. Does this return bogus CEL
> data, trigger KASAN, crash the test module, or something else? If there
> is a demonstrated failure, please describe it. Otherwise, I think the
> property being restored is the more important aspect to capture in the
> commit log.

I am getting ready to travel and I will address this when I return in
about two weeks. Thank you for understanding.