[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

[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.

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.

A longer explanation appears below, but the bug can cause the kernel
to panic with an out-of-bounds copy. So highlight that fact in the
commit message?

---

The bug was validated with a PoC that deliberately invalidates the CXL
mailbox GET_LOG command to the mock CXL memory device.

The vulnerable code checked the requested log slice like this:

  if (offset + length > sizeof(mock_cel))
  return -EINVAL;

Both offset and length are 32-bit fields supplied by userspace. The PoC sets:

  in.offset = UINT32_MAX;
  in.length = 1;

So the vulnerable expression wraps:

  UINT32_MAX + 1 == 0

That makes the range check pass, because 0 > sizeof(mock_cel) is false.

After that, the mock GET_LOG handler still uses the original huge offset:

  memcpy(cmd->payload_out, data + offset, length);

So it tries to copy 1 byte from far past the mock CEL buffer. In QEMU
pre-fix, that caused a kernel page fault in memcpy_orig, through
cxl_mock_mbox_send, and then a panic because the test kernel used
oops=panic.

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

[Richard Cheng]

On Sun, Jun 28, 2026 at 11:45:29AM +0800, Samuel Moelius wrote:
> 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.
> 
> A longer explanation appears below, but the bug can cause the kernel
> to panic with an out-of-bounds copy. So highlight that fact in the
> commit message?
> 
> ---
> 
> The bug was validated with a PoC that deliberately invalidates the CXL
> mailbox GET_LOG command to the mock CXL memory device.
> 
> The vulnerable code checked the requested log slice like this:
> 
>   if (offset + length > sizeof(mock_cel))
>   return -EINVAL;
> 
> Both offset and length are 32-bit fields supplied by userspace. The PoC sets:
> 
>   in.offset = UINT32_MAX;
>   in.length = 1;
> 
> So the vulnerable expression wraps:
> 
>   UINT32_MAX + 1 == 0
> 
> That makes the range check pass, because 0 > sizeof(mock_cel) is false.
> 
> After that, the mock GET_LOG handler still uses the original huge offset:
> 
>   memcpy(cmd->payload_out, data + offset, length);
> 
> So it tries to copy 1 byte from far past the mock CEL buffer. In QEMU
> pre-fix, that caused a kernel page fault in memcpy_orig, through
> cxl_mock_mbox_send, and then a panic because the test kernel used
> oops=panic.
>

Hi Samuel,

The substrction range check looks right to me, it also matches the existing mock
LSA checks.

I would suggest a small regression test if you have time for it.
Now in-mind only a pseudo-code version, maybe something like that following

"""
/* Use a cxl_test memdev and the valid CEL UUID/advertised CEL size. */
ASSERT_EQ(send_get_log(fd, cel_uuid, UINT32_MAX, 1, out, 1), -EINVAL);

/* Preserve valid boundaries. */
ASSERT_EQ(send_get_log(fd, cel_uuid, cel_size - 1, 1, out, 1), 0);
ASSERT_EQ(send_get_log(fd, cel_uuid, cel_size, 0, NULL, 0), 0);
"""

The first case captures the overflow regression, the latter 2 document the
intended inclusive end-of-buffer behavior.

What do you think ?

Best regards,
Richard Cheng. 

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

[Samuel Moelius]

On Tue, Jun 30, 2026 at 5:27 AM Richard Cheng <icheng@nvidia.com> wrote:
>
> On Sun, Jun 28, 2026 at 11:45:29AM +0800, Samuel Moelius wrote:
> > 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.
> >
> > A longer explanation appears below, but the bug can cause the kernel
> > to panic with an out-of-bounds copy. So highlight that fact in the
> > commit message?
> >
> > ---
> >
> > The bug was validated with a PoC that deliberately invalidates the CXL
> > mailbox GET_LOG command to the mock CXL memory device.
> >
> > The vulnerable code checked the requested log slice like this:
> >
> >   if (offset + length > sizeof(mock_cel))
> >   return -EINVAL;
> >
> > Both offset and length are 32-bit fields supplied by userspace. The PoC sets:
> >
> >   in.offset = UINT32_MAX;
> >   in.length = 1;
> >
> > So the vulnerable expression wraps:
> >
> >   UINT32_MAX + 1 == 0
> >
> > That makes the range check pass, because 0 > sizeof(mock_cel) is false.
> >
> > After that, the mock GET_LOG handler still uses the original huge offset:
> >
> >   memcpy(cmd->payload_out, data + offset, length);
> >
> > So it tries to copy 1 byte from far past the mock CEL buffer. In QEMU
> > pre-fix, that caused a kernel page fault in memcpy_orig, through
> > cxl_mock_mbox_send, and then a panic because the test kernel used
> > oops=panic.
> >
>
> Hi Samuel,
>
> The substrction range check looks right to me, it also matches the existing mock
> LSA checks.
>
> I would suggest a small regression test if you have time for it.
> Now in-mind only a pseudo-code version, maybe something like that following
>
> """
> /* Use a cxl_test memdev and the valid CEL UUID/advertised CEL size. */
> ASSERT_EQ(send_get_log(fd, cel_uuid, UINT32_MAX, 1, out, 1), -EINVAL);
>
> /* Preserve valid boundaries. */
> ASSERT_EQ(send_get_log(fd, cel_uuid, cel_size - 1, 1, out, 1), 0);
> ASSERT_EQ(send_get_log(fd, cel_uuid, cel_size, 0, NULL, 0), 0);
> """
>
> The first case captures the overflow regression, the latter 2 document the
> intended inclusive end-of-buffer behavior.
>
> What do you think ?

Could you please give me some more specifics of the type of test you
imagine? For example, should it be a completely new test? Or should it
build on some existing CXL test?

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

[Richard Cheng]

On Wed, Jul 01, 2026 at 08:38:54PM +0800, Samuel Moelius wrote:
> On Tue, Jun 30, 2026 at 5:27 AM Richard Cheng <icheng@nvidia.com> wrote:
> >
> > On Sun, Jun 28, 2026 at 11:45:29AM +0800, Samuel Moelius wrote:
> > > 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.
> > >
> > > A longer explanation appears below, but the bug can cause the kernel
> > > to panic with an out-of-bounds copy. So highlight that fact in the
> > > commit message?
> > >
> > > ---
> > >
> > > The bug was validated with a PoC that deliberately invalidates the CXL
> > > mailbox GET_LOG command to the mock CXL memory device.
> > >
> > > The vulnerable code checked the requested log slice like this:
> > >
> > >   if (offset + length > sizeof(mock_cel))
> > >   return -EINVAL;
> > >
> > > Both offset and length are 32-bit fields supplied by userspace. The PoC sets:
> > >
> > >   in.offset = UINT32_MAX;
> > >   in.length = 1;
> > >
> > > So the vulnerable expression wraps:
> > >
> > >   UINT32_MAX + 1 == 0
> > >
> > > That makes the range check pass, because 0 > sizeof(mock_cel) is false.
> > >
> > > After that, the mock GET_LOG handler still uses the original huge offset:
> > >
> > >   memcpy(cmd->payload_out, data + offset, length);
> > >
> > > So it tries to copy 1 byte from far past the mock CEL buffer. In QEMU
> > > pre-fix, that caused a kernel page fault in memcpy_orig, through
> > > cxl_mock_mbox_send, and then a panic because the test kernel used
> > > oops=panic.
> > >
> >
> > Hi Samuel,
> >
> > The substrction range check looks right to me, it also matches the existing mock
> > LSA checks.
> >
> > I would suggest a small regression test if you have time for it.
> > Now in-mind only a pseudo-code version, maybe something like that following
> >
> > """
> > /* Use a cxl_test memdev and the valid CEL UUID/advertised CEL size. */
> > ASSERT_EQ(send_get_log(fd, cel_uuid, UINT32_MAX, 1, out, 1), -EINVAL);
> >
> > /* Preserve valid boundaries. */
> > ASSERT_EQ(send_get_log(fd, cel_uuid, cel_size - 1, 1, out, 1), 0);
> > ASSERT_EQ(send_get_log(fd, cel_uuid, cel_size, 0, NULL, 0), 0);
> > """
> >
> > The first case captures the overflow regression, the latter 2 document the
> > intended inclusive end-of-buffer behavior.
> >
> > What do you think ?
> 
> Could you please give me some more specifics of the type of test you
> imagine? For example, should it be a completely new test? Or should it
> build on some existing CXL test?

Hi Samuel,

I'm thinking about adding it on ndctl's test/cxl-mbox.c . It already opens
/dev/cxl/<memdev> and issues raw CXL_MEM_SEND_COMMAND against the cxl_test.

Something like the following

"""
    /* struct cxl_mbox_get_log: uuid[16] + __le32 offset + __le32 length */
    struct get_log_in {
            __u8  uuid[16];
            __le32 offset;
            __le32 length;
    } __attribute__((packed));

    /* CEL UUID da9c0b5-bf41-4b78-8f79-96b1623b3f17, GUID byte order */
    static const __u8 cel_uuid[16] = {
            0xb5,0xc0,0xa9,0x0d, 0x41,0xbf, 0x78,0x4b,
            0x8f,0x79, 0x96,0xb1,0x62,0x3b,0x3f,0x17,
    };

    struct get_log_in in = { .offset = cpu_to_le32(0xffffffffU),
                             .length = cpu_to_le32(1) };
    memcpy(in.uuid, cel_uuid, sizeof(in.uuid));

    struct cxl_send_command c = {
            .id = CXL_MEM_COMMAND_ID_GET_LOG,
            .in  = { .size = sizeof(in), .payload = (__u64)(uintptr_t)&in },
            .out = { .size = 1,          .payload = (__u64)(uintptr_t)out },
    };
    rc = ioctl(fd, CXL_MEM_SEND_COMMAND, &c);
    /* Pre-fix: offset+length wraps to 0, passes the bounds check, and the
     * mock memcpy()s from data + 0xffffffff -> OOB read / panic.
     * Post-fix: reject. Assert the request is refused. */
    ASSERT(rc < 0 || c.retval != 0);   /* mock returns -EINVAL -> retval set */
"""

This is draft by my claude-code, I haven't test it yet, but the idea seems
correct.
Note that the UUID has to be the valid CEL UUID.

I'm happy to send thest test as a follow-up ndctl patch on top of your fix, or
you would prefer to fold it in, whichever you like.

--Richard

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

[Samuel Moelius]

On Fri, Jul 3, 2026 at 4:39 AM Richard Cheng <icheng@nvidia.com> wrote:
>
> On Wed, Jul 01, 2026 at 08:38:54PM +0800, Samuel Moelius wrote:
> > On Tue, Jun 30, 2026 at 5:27 AM Richard Cheng <icheng@nvidia.com> wrote:
> > >
> > > On Sun, Jun 28, 2026 at 11:45:29AM +0800, Samuel Moelius wrote:
> > > > 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.
> > > >
> > > > A longer explanation appears below, but the bug can cause the kernel
> > > > to panic with an out-of-bounds copy. So highlight that fact in the
> > > > commit message?
> > > >
> > > > ---
> > > >
> > > > The bug was validated with a PoC that deliberately invalidates the CXL
> > > > mailbox GET_LOG command to the mock CXL memory device.
> > > >
> > > > The vulnerable code checked the requested log slice like this:
> > > >
> > > >   if (offset + length > sizeof(mock_cel))
> > > >   return -EINVAL;
> > > >
> > > > Both offset and length are 32-bit fields supplied by userspace. The PoC sets:
> > > >
> > > >   in.offset = UINT32_MAX;
> > > >   in.length = 1;
> > > >
> > > > So the vulnerable expression wraps:
> > > >
> > > >   UINT32_MAX + 1 == 0
> > > >
> > > > That makes the range check pass, because 0 > sizeof(mock_cel) is false.
> > > >
> > > > After that, the mock GET_LOG handler still uses the original huge offset:
> > > >
> > > >   memcpy(cmd->payload_out, data + offset, length);
> > > >
> > > > So it tries to copy 1 byte from far past the mock CEL buffer. In QEMU
> > > > pre-fix, that caused a kernel page fault in memcpy_orig, through
> > > > cxl_mock_mbox_send, and then a panic because the test kernel used
> > > > oops=panic.
> > > >
> > >
> > > Hi Samuel,
> > >
> > > The substrction range check looks right to me, it also matches the existing mock
> > > LSA checks.
> > >
> > > I would suggest a small regression test if you have time for it.
> > > Now in-mind only a pseudo-code version, maybe something like that following
> > >
> > > """
> > > /* Use a cxl_test memdev and the valid CEL UUID/advertised CEL size. */
> > > ASSERT_EQ(send_get_log(fd, cel_uuid, UINT32_MAX, 1, out, 1), -EINVAL);
> > >
> > > /* Preserve valid boundaries. */
> > > ASSERT_EQ(send_get_log(fd, cel_uuid, cel_size - 1, 1, out, 1), 0);
> > > ASSERT_EQ(send_get_log(fd, cel_uuid, cel_size, 0, NULL, 0), 0);
> > > """
> > >
> > > The first case captures the overflow regression, the latter 2 document the
> > > intended inclusive end-of-buffer behavior.
> > >
> > > What do you think ?
> >
> > Could you please give me some more specifics of the type of test you
> > imagine? For example, should it be a completely new test? Or should it
> > build on some existing CXL test?
>
> Hi Samuel,
>
> I'm thinking about adding it on ndctl's test/cxl-mbox.c . It already opens
> /dev/cxl/<memdev> and issues raw CXL_MEM_SEND_COMMAND against the cxl_test.
>
> Something like the following
>
> """
>     /* struct cxl_mbox_get_log: uuid[16] + __le32 offset + __le32 length */
>     struct get_log_in {
>             __u8  uuid[16];
>             __le32 offset;
>             __le32 length;
>     } __attribute__((packed));
>
>     /* CEL UUID da9c0b5-bf41-4b78-8f79-96b1623b3f17, GUID byte order */
>     static const __u8 cel_uuid[16] = {
>             0xb5,0xc0,0xa9,0x0d, 0x41,0xbf, 0x78,0x4b,
>             0x8f,0x79, 0x96,0xb1,0x62,0x3b,0x3f,0x17,
>     };
>
>     struct get_log_in in = { .offset = cpu_to_le32(0xffffffffU),
>                              .length = cpu_to_le32(1) };
>     memcpy(in.uuid, cel_uuid, sizeof(in.uuid));
>
>     struct cxl_send_command c = {
>             .id = CXL_MEM_COMMAND_ID_GET_LOG,
>             .in  = { .size = sizeof(in), .payload = (__u64)(uintptr_t)&in },
>             .out = { .size = 1,          .payload = (__u64)(uintptr_t)out },
>     };
>     rc = ioctl(fd, CXL_MEM_SEND_COMMAND, &c);
>     /* Pre-fix: offset+length wraps to 0, passes the bounds check, and the
>      * mock memcpy()s from data + 0xffffffff -> OOB read / panic.
>      * Post-fix: reject. Assert the request is refused. */
>     ASSERT(rc < 0 || c.retval != 0);   /* mock returns -EINVAL -> retval set */
> """
>
> This is draft by my claude-code, I haven't test it yet, but the idea seems
> correct.
> Note that the UUID has to be the valid CEL UUID.
>
> I'm happy to send thest test as a follow-up ndctl patch on top of your fix, or
> you would prefer to fold it in, whichever you like.

I would like to please take you up on the follow-up patch to ndctl, if
you don't mind.

Thank you.