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

[Samuel Moelius]

On Fri, Jul 3, 2026 at 4:39 AM Richard Cheng <icheng@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > > > >
> > > > > 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.