[PATCH] hw/cxl: bound remaining Set Feature writes

[PATCH] hw/cxl: bound remaining Set Feature writes

[Jia Jia]

Commit c1c4d6b38b13 added offset + length checks for the
patrol_scrub and ecs Set Feature branches, but the remaining
branches still copy mailbox payload data into fixed-size
write-attribute objects without the same validation.

A full mailbox payload can still reach rank_sparing and overrun
CXLMemSparingWriteAttrs on current master. With an ASan build
this aborts the host process with:

  ERROR: AddressSanitizer: heap-buffer-overflow
  WRITE of size 2016
      #0 __interceptor_memcpy
      #1 cmd_features_set_feature ../hw/cxl/cxl-mailbox-utils.c:1908
      #2 cxl_process_cci_message ../hw/cxl/cxl-mailbox-utils.c:4622
      #3 mailbox_reg_write ../hw/cxl/cxl-device-utils.c:209

Apply the same offset + length validation to soft_ppr,
hard_ppr, cacheline_sparing, row_sparing, bank_sparing, and
rank_sparing so oversized requests fail with
CXL_MBOX_INVALID_PAYLOAD_LENGTH instead of overflowing the
write-attribute buffers.

Add a qtest covering the rank_sparing path.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3458
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
 hw/cxl/cxl-mailbox-utils.c | 20 ++++++++
 tests/qtest/cxl-test.c     | 99 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index d8ba7e8625..ce139e30eb 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1713,6 +1713,7 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
     CXLSetFeatureInHeader *hdr = (void *)payload_in;
     CXLSetFeatureInfo *set_feat_info;
     uint16_t bytes_to_copy = 0;
+    uint32_t end_offset;
     uint8_t data_transfer_flag;
     CXLType3Dev *ct3d;
     uint16_t count;
@@ -1746,6 +1747,7 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
     set_feat_info->data_transfer_flag = data_transfer_flag;
     set_feat_info->data_offset = hdr->offset;
     bytes_to_copy = len_in - sizeof(CXLSetFeatureInHeader);
+    end_offset = (uint32_t)hdr->offset + bytes_to_copy;
 
     if (bytes_to_copy == 0) {
         return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
@@ -1813,6 +1815,9 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
+        if (end_offset > sizeof(ct3d->soft_ppr_wr_attrs)) {
+            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+        }
         memcpy((uint8_t *)&ct3d->soft_ppr_wr_attrs + hdr->offset,
                sppr_write_attrs, bytes_to_copy);
         set_feat_info->data_size += bytes_to_copy;
@@ -1832,6 +1837,9 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
+        if (end_offset > sizeof(ct3d->hard_ppr_wr_attrs)) {
+            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+        }
         memcpy((uint8_t *)&ct3d->hard_ppr_wr_attrs + hdr->offset,
                hppr_write_attrs, bytes_to_copy);
         set_feat_info->data_size += bytes_to_copy;
@@ -1851,6 +1859,9 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
+        if (end_offset > sizeof(ct3d->cacheline_sparing_wr_attrs)) {
+            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+        }
         memcpy((uint8_t *)&ct3d->cacheline_sparing_wr_attrs + hdr->offset,
                mem_sparing_write_attrs, bytes_to_copy);
         set_feat_info->data_size += bytes_to_copy;
@@ -1869,6 +1880,9 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
+        if (end_offset > sizeof(ct3d->row_sparing_wr_attrs)) {
+            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+        }
         memcpy((uint8_t *)&ct3d->row_sparing_wr_attrs + hdr->offset,
                mem_sparing_write_attrs, bytes_to_copy);
         set_feat_info->data_size += bytes_to_copy;
@@ -1887,6 +1901,9 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
+        if (end_offset > sizeof(ct3d->bank_sparing_wr_attrs)) {
+            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+        }
         memcpy((uint8_t *)&ct3d->bank_sparing_wr_attrs + hdr->offset,
                mem_sparing_write_attrs, bytes_to_copy);
         set_feat_info->data_size += bytes_to_copy;
@@ -1905,6 +1922,9 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
+        if (end_offset > sizeof(ct3d->rank_sparing_wr_attrs)) {
+            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+        }
         memcpy((uint8_t *)&ct3d->rank_sparing_wr_attrs + hdr->offset,
                mem_sparing_write_attrs, bytes_to_copy);
         set_feat_info->data_size += bytes_to_copy;
diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
index 8fb7e58d4f..a9fcd98736 100644
--- a/tests/qtest/cxl-test.c
+++ b/tests/qtest/cxl-test.c
@@ -7,6 +7,7 @@
 
 #include "qemu/osdep.h"
 #include "libqtest-single.h"
+#include "hw/cxl/cxl_device.h"
 
 #define QEMU_PXB_CMD \
     "-machine q35,cxl=on " \
@@ -59,6 +60,12 @@
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
     "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,lsa=lsa0,id=mem0 "
 
+#define QEMU_T3D_DIRECT_PMEM \
+    "-machine q35,cxl=on -nodefaults " \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=1M " \
+    "-device cxl-type3,bus=pcie.0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 "
+
 #define QEMU_2T3D \
     "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
@@ -81,6 +88,17 @@
     "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M " \
     "-device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3,id=pmem3 "
 
+#define CXL_T3D_DEVFN 0x08
+#define CXL_T3D_BAR2_ADDR 0x10000000ULL
+
+typedef struct QEMU_PACKED CXLSetFeatureInHeaderTest {
+    uint8_t uuid[16];
+    uint32_t flags;
+    uint16_t offset;
+    uint8_t version;
+    uint8_t rsvd[9];
+} CXLSetFeatureInHeaderTest;
+
 static void cxl_basic_hb(void)
 {
     qtest_start("-machine q35,cxl=on");
@@ -118,6 +136,85 @@ static void cxl_2root_port(void)
 }
 
 #ifdef CONFIG_POSIX
+static uint32_t cxl_test_pci_config_addr(uint8_t devfn, uint8_t offset)
+{
+    return 0x80000000U | (devfn << 8) | offset;
+}
+
+static void cxl_test_t3d_enable_bar2(void)
+{
+    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x18));
+    outl(0xcfc, CXL_T3D_BAR2_ADDR);
+    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x1c));
+    outl(0xcfc, 0);
+    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x04));
+    outl(0xcfc, 0x2);
+}
+
+static uint64_t cxl_test_t3d_mailbox_base(void)
+{
+    return CXL_T3D_BAR2_ADDR + CXL_MAILBOX_REGISTERS_OFFSET;
+}
+
+static uint64_t cxl_test_t3d_payload_base(void)
+{
+    return cxl_test_t3d_mailbox_base() + A_CXL_DEV_CMD_PAYLOAD;
+}
+
+static void cxl_test_t3d_submit_set_feature(const void *payload, size_t len)
+{
+    memwrite(cxl_test_t3d_payload_base(), payload, len);
+    writeq(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_CMD,
+           ((uint64_t)len << 16) | (0x05 << 8) | 0x02);
+    writel(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_CTRL, 1);
+}
+
+static uint16_t cxl_test_t3d_mailbox_errno(void)
+{
+    return (readq(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_STS) >>
+            32) & 0xffff;
+}
+
+static void cxl_test_fill_set_feature_header(CXLSetFeatureInHeaderTest *hdr,
+                                             const uint8_t uuid[16],
+                                             uint16_t offset,
+                                             uint8_t version)
+{
+    memset(hdr, 0, sizeof(*hdr));
+    memcpy(hdr->uuid, uuid, 16);
+    hdr->offset = cpu_to_le16(offset);
+    hdr->version = version;
+}
+
+static void cxl_t3d_set_feature_rejects_oversized_rank_sparing(void)
+{
+    static const uint8_t rank_sparing_uuid[16] = {
+        0x34, 0xdb, 0xaf, 0xf5, 0x05, 0x52, 0x42, 0x81,
+        0x8f, 0x76, 0xda, 0x0b, 0x5e, 0x7a, 0x76, 0xa7,
+    };
+    g_autoptr(GString) cmdline = g_string_new(NULL);
+    g_autofree const char *tmpfs = NULL;
+    uint8_t payload[CXL_MAILBOX_MAX_PAYLOAD_SIZE] = { 0 };
+    CXLSetFeatureInHeaderTest *hdr = (void *)payload;
+
+    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
+    g_string_printf(cmdline, QEMU_T3D_DIRECT_PMEM, tmpfs, tmpfs);
+
+    qtest_start(cmdline->str);
+    cxl_test_t3d_enable_bar2();
+
+    cxl_test_fill_set_feature_header(hdr, rank_sparing_uuid, 0,
+                                     CXL_MEMDEV_SPARING_SET_FEATURE_VERSION);
+    memset(payload + sizeof(*hdr), 0x41,
+           sizeof(payload) - sizeof(*hdr));
+    cxl_test_t3d_submit_set_feature(payload, sizeof(payload));
+    g_assert_cmphex(cxl_test_t3d_mailbox_errno(), ==,
+                    CXL_MBOX_INVALID_PAYLOAD_LENGTH);
+
+    qtest_end();
+    rmdir(tmpfs);
+}
+
 static void cxl_t3d_deprecated(void)
 {
     g_autoptr(GString) cmdline = g_string_new(NULL);
@@ -238,6 +335,8 @@ int main(int argc, char **argv)
         qtest_add_func("/pci/cxl/type3_device_pmem", cxl_t3d_persistent);
         qtest_add_func("/pci/cxl/type3_device_vmem", cxl_t3d_volatile);
         qtest_add_func("/pci/cxl/type3_device_vmem_lsa", cxl_t3d_volatile_lsa);
+        qtest_add_func("/pci/cxl/type3_device_set_feature_rank_sparing_bounds",
+                       cxl_t3d_set_feature_rejects_oversized_rank_sparing);
         qtest_add_func("/pci/cxl/rp_x2_type3_x2", cxl_1pxb_2rp_2t3d);
         qtest_add_func("/pci/cxl/pxb_x2_root_port_x4_type3_x4",
                        cxl_2pxb_4rp_4t3d);
-- 
2.34.1

Re: [PATCH] hw/cxl: bound remaining Set Feature writes

[Peter Maydell]

On Wed, 29 Apr 2026 at 13:46, Jia Jia <physicalmtea@gmail.com> wrote:
>
> Commit c1c4d6b38b13 added offset + length checks for the
> patrol_scrub and ecs Set Feature branches, but the remaining
> branches still copy mailbox payload data into fixed-size
> write-attribute objects without the same validation.
>
> A full mailbox payload can still reach rank_sparing and overrun
> CXLMemSparingWriteAttrs on current master. With an ASan build
> this aborts the host process with:
>
>   ERROR: AddressSanitizer: heap-buffer-overflow
>   WRITE of size 2016
>       #0 __interceptor_memcpy
>       #1 cmd_features_set_feature ../hw/cxl/cxl-mailbox-utils.c:1908
>       #2 cxl_process_cci_message ../hw/cxl/cxl-mailbox-utils.c:4622
>       #3 mailbox_reg_write ../hw/cxl/cxl-device-utils.c:209
>
> Apply the same offset + length validation to soft_ppr,
> hard_ppr, cacheline_sparing, row_sparing, bank_sparing, and
> rank_sparing so oversized requests fail with
> CXL_MBOX_INVALID_PAYLOAD_LENGTH instead of overflowing the
> write-attribute buffers.
>
> Add a qtest covering the rank_sparing path.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3458
> Signed-off-by: Jia Jia <physicalmtea@gmail.com>

I don't think we should mix refactoring with this bug fix,
but I do notice there's a lot of very repetitive looking
code in this function, which is why the patch has to add a
length check in six different places, and any new feature
type will have to add another one. maybe there's a way to make
it less awkwardly repetitive...

thanks
-- PMM

[PATCH v2] hw/cxl: bound Set Feature writes

[Jia Jia]

Commit c1c4d6b38b13 added offset + length checks for the
patrol_scrub and ecs Set Feature branches, but the remaining
branches still copy mailbox payload data into fixed-size
write-attribute objects without the same validation.

A full mailbox payload can still reach rank_sparing and overrun
CXLMemSparingWriteAttrs on current master. With an ASan build
this aborts the host process with:

  ERROR: AddressSanitizer: heap-buffer-overflow
  WRITE of size 2016
      #0 __interceptor_memcpy
      #1 cmd_features_set_feature ../hw/cxl/cxl-mailbox-utils.c:1908
      #2 cxl_process_cci_message ../hw/cxl/cxl-mailbox-utils.c:4622
      #3 mailbox_reg_write ../hw/cxl/cxl-device-utils.c:209

Fold the bounds checking into a small helper and use it for
all Set Feature write-attribute branches, so oversized
requests fail with CXL_MBOX_INVALID_PAYLOAD_LENGTH instead
of overflowing the target buffers.

Add a qtest covering the rank_sparing path.

Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3458
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
Hi Peter,

Thanks, that makes sense.

I've folded the repeated bounds checking into a small helper and respun
the patch as v2.

Thanks

v2:
- fold the repeated Set Feature bounds checks into a helper
- use the helper for all Set Feature write-attribute branches

 hw/cxl/cxl-mailbox-utils.c | 94 ++++++++++++++++++++++++------
 tests/qtest/cxl-test.c     | 99 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 169 insertions(+), 24 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index d8ba7e8625..4c7a083e4c 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1702,6 +1702,21 @@ static CXLRetCode cmd_features_get_feature(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+static CXLRetCode cxl_set_feature_copy(void *write_attrs,
+                                       size_t write_attrs_size,
+                                       uint16_t offset,
+                                       const void *payload,
+                                       uint16_t bytes_to_copy)
+{
+    if ((uint32_t)offset + bytes_to_copy > write_attrs_size) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
+    memcpy((uint8_t *)write_attrs + offset, payload, bytes_to_copy);
+
+    return CXL_MBOX_SUCCESS;
+}
+
 /* CXL r3.1 section 8.2.9.6.3: Set Feature (Opcode 0502h) */
 static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
                                            uint8_t *payload_in,
@@ -1713,6 +1728,7 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
     CXLSetFeatureInHeader *hdr = (void *)payload_in;
     CXLSetFeatureInfo *set_feat_info;
     uint16_t bytes_to_copy = 0;
+    CXLRetCode ret;
     uint8_t data_transfer_flag;
     CXLType3Dev *ct3d;
     uint16_t count;
@@ -1760,13 +1776,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        if ((uint32_t)hdr->offset + bytes_to_copy >
-            sizeof(ct3d->patrol_scrub_wr_attrs)) {
-            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
-        }
-        memcpy((uint8_t *)&ct3d->patrol_scrub_wr_attrs + hdr->offset,
-               ps_write_attrs,
-               bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->patrol_scrub_wr_attrs,
+                                   sizeof(ct3d->patrol_scrub_wr_attrs),
+                                   hdr->offset, ps_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1787,13 +1803,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        if ((uint32_t)hdr->offset + bytes_to_copy >
-            sizeof(ct3d->ecs_wr_attrs)) {
-            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
-        }
-        memcpy((uint8_t *)&ct3d->ecs_wr_attrs + hdr->offset,
-               ecs_write_attrs,
-               bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->ecs_wr_attrs,
+                                   sizeof(ct3d->ecs_wr_attrs),
+                                   hdr->offset, ecs_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1813,8 +1829,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->soft_ppr_wr_attrs + hdr->offset,
-               sppr_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->soft_ppr_wr_attrs,
+                                   sizeof(ct3d->soft_ppr_wr_attrs),
+                                   hdr->offset, sppr_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1832,8 +1853,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->hard_ppr_wr_attrs + hdr->offset,
-               hppr_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->hard_ppr_wr_attrs,
+                                   sizeof(ct3d->hard_ppr_wr_attrs),
+                                   hdr->offset, hppr_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1851,8 +1877,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->cacheline_sparing_wr_attrs + hdr->offset,
-               mem_sparing_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->cacheline_sparing_wr_attrs,
+                                   sizeof(ct3d->cacheline_sparing_wr_attrs),
+                                   hdr->offset, mem_sparing_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1869,8 +1900,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->row_sparing_wr_attrs + hdr->offset,
-               mem_sparing_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->row_sparing_wr_attrs,
+                                   sizeof(ct3d->row_sparing_wr_attrs),
+                                   hdr->offset, mem_sparing_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1887,8 +1923,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->bank_sparing_wr_attrs + hdr->offset,
-               mem_sparing_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->bank_sparing_wr_attrs,
+                                   sizeof(ct3d->bank_sparing_wr_attrs),
+                                   hdr->offset, mem_sparing_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1905,8 +1946,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->rank_sparing_wr_attrs + hdr->offset,
-               mem_sparing_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->rank_sparing_wr_attrs,
+                                   sizeof(ct3d->rank_sparing_wr_attrs),
+                                   hdr->offset, mem_sparing_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
             data_transfer_flag == CXL_SET_FEATURE_FLAG_FINISH_DATA_TRANSFER) {
diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
index 8fb7e58d4f..a9fcd98736 100644
--- a/tests/qtest/cxl-test.c
+++ b/tests/qtest/cxl-test.c
@@ -7,6 +7,7 @@
 
 #include "qemu/osdep.h"
 #include "libqtest-single.h"
+#include "hw/cxl/cxl_device.h"
 
 #define QEMU_PXB_CMD \
     "-machine q35,cxl=on " \
@@ -59,6 +60,12 @@
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
     "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,lsa=lsa0,id=mem0 "
 
+#define QEMU_T3D_DIRECT_PMEM \
+    "-machine q35,cxl=on -nodefaults " \
+    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
+    "-object memory-backend-file,id=lsa0,mem-path=%s,size=1M " \
+    "-device cxl-type3,bus=pcie.0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 "
+
 #define QEMU_2T3D \
     "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
     "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
@@ -81,6 +88,17 @@
     "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M " \
     "-device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3,id=pmem3 "
 
+#define CXL_T3D_DEVFN 0x08
+#define CXL_T3D_BAR2_ADDR 0x10000000ULL
+
+typedef struct QEMU_PACKED CXLSetFeatureInHeaderTest {
+    uint8_t uuid[16];
+    uint32_t flags;
+    uint16_t offset;
+    uint8_t version;
+    uint8_t rsvd[9];
+} CXLSetFeatureInHeaderTest;
+
 static void cxl_basic_hb(void)
 {
     qtest_start("-machine q35,cxl=on");
@@ -118,6 +136,85 @@ static void cxl_2root_port(void)
 }
 
 #ifdef CONFIG_POSIX
+static uint32_t cxl_test_pci_config_addr(uint8_t devfn, uint8_t offset)
+{
+    return 0x80000000U | (devfn << 8) | offset;
+}
+
+static void cxl_test_t3d_enable_bar2(void)
+{
+    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x18));
+    outl(0xcfc, CXL_T3D_BAR2_ADDR);
+    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x1c));
+    outl(0xcfc, 0);
+    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x04));
+    outl(0xcfc, 0x2);
+}
+
+static uint64_t cxl_test_t3d_mailbox_base(void)
+{
+    return CXL_T3D_BAR2_ADDR + CXL_MAILBOX_REGISTERS_OFFSET;
+}
+
+static uint64_t cxl_test_t3d_payload_base(void)
+{
+    return cxl_test_t3d_mailbox_base() + A_CXL_DEV_CMD_PAYLOAD;
+}
+
+static void cxl_test_t3d_submit_set_feature(const void *payload, size_t len)
+{
+    memwrite(cxl_test_t3d_payload_base(), payload, len);
+    writeq(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_CMD,
+           ((uint64_t)len << 16) | (0x05 << 8) | 0x02);
+    writel(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_CTRL, 1);
+}
+
+static uint16_t cxl_test_t3d_mailbox_errno(void)
+{
+    return (readq(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_STS) >>
+            32) & 0xffff;
+}
+
+static void cxl_test_fill_set_feature_header(CXLSetFeatureInHeaderTest *hdr,
+                                             const uint8_t uuid[16],
+                                             uint16_t offset,
+                                             uint8_t version)
+{
+    memset(hdr, 0, sizeof(*hdr));
+    memcpy(hdr->uuid, uuid, 16);
+    hdr->offset = cpu_to_le16(offset);
+    hdr->version = version;
+}
+
+static void cxl_t3d_set_feature_rejects_oversized_rank_sparing(void)
+{
+    static const uint8_t rank_sparing_uuid[16] = {
+        0x34, 0xdb, 0xaf, 0xf5, 0x05, 0x52, 0x42, 0x81,
+        0x8f, 0x76, 0xda, 0x0b, 0x5e, 0x7a, 0x76, 0xa7,
+    };
+    g_autoptr(GString) cmdline = g_string_new(NULL);
+    g_autofree const char *tmpfs = NULL;
+    uint8_t payload[CXL_MAILBOX_MAX_PAYLOAD_SIZE] = { 0 };
+    CXLSetFeatureInHeaderTest *hdr = (void *)payload;
+
+    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
+    g_string_printf(cmdline, QEMU_T3D_DIRECT_PMEM, tmpfs, tmpfs);
+
+    qtest_start(cmdline->str);
+    cxl_test_t3d_enable_bar2();
+
+    cxl_test_fill_set_feature_header(hdr, rank_sparing_uuid, 0,
+                                     CXL_MEMDEV_SPARING_SET_FEATURE_VERSION);
+    memset(payload + sizeof(*hdr), 0x41,
+           sizeof(payload) - sizeof(*hdr));
+    cxl_test_t3d_submit_set_feature(payload, sizeof(payload));
+    g_assert_cmphex(cxl_test_t3d_mailbox_errno(), ==,
+                    CXL_MBOX_INVALID_PAYLOAD_LENGTH);
+
+    qtest_end();
+    rmdir(tmpfs);
+}
+
 static void cxl_t3d_deprecated(void)
 {
     g_autoptr(GString) cmdline = g_string_new(NULL);
@@ -238,6 +335,8 @@ int main(int argc, char **argv)
         qtest_add_func("/pci/cxl/type3_device_pmem", cxl_t3d_persistent);
         qtest_add_func("/pci/cxl/type3_device_vmem", cxl_t3d_volatile);
         qtest_add_func("/pci/cxl/type3_device_vmem_lsa", cxl_t3d_volatile_lsa);
+        qtest_add_func("/pci/cxl/type3_device_set_feature_rank_sparing_bounds",
+                       cxl_t3d_set_feature_rejects_oversized_rank_sparing);
         qtest_add_func("/pci/cxl/rp_x2_type3_x2", cxl_1pxb_2rp_2t3d);
         qtest_add_func("/pci/cxl/pxb_x2_root_port_x4_type3_x4",
                        cxl_2pxb_4rp_4t3d);
-- 
2.34.1

Re: [PATCH v2] hw/cxl: bound Set Feature writes

[Jonathan Cameron]

On Wed, 29 Apr 2026 23:27:50 +0800
Jia Jia <physicalmtea@gmail.com> wrote:

Sorry I've taken so long to get to cxl qemu stuff.

> Commit c1c4d6b38b13 added offset + length checks for the
> patrol_scrub and ecs Set Feature branches, but the remaining
> branches still copy mailbox payload data into fixed-size
> write-attribute objects without the same validation.
> 
> A full mailbox payload can still reach rank_sparing and overrun
> CXLMemSparingWriteAttrs on current master. With an ASan build
> this aborts the host process with:
> 
>   ERROR: AddressSanitizer: heap-buffer-overflow
>   WRITE of size 2016
>       #0 __interceptor_memcpy
>       #1 cmd_features_set_feature ../hw/cxl/cxl-mailbox-utils.c:1908
>       #2 cxl_process_cci_message ../hw/cxl/cxl-mailbox-utils.c:4622
>       #3 mailbox_reg_write ../hw/cxl/cxl-device-utils.c:209
> 
> Fold the bounds checking into a small helper and use it for
> all Set Feature write-attribute branches, so oversized
> requests fail with CXL_MBOX_INVALID_PAYLOAD_LENGTH instead
> of overflowing the target buffers.
> 
> Add a qtest covering the rank_sparing path.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3458

Please can we have a Fixes tag (possibly several) for where the bug was introduced.

This is great in general - comments are all about splitting it up
and minor things to make the test easier to understand.

Thanks

Jonathan


> Signed-off-by: Jia Jia <physicalmtea@gmail.com>
> ---
> Hi Peter,
> 
> Thanks, that makes sense.
> 
> I've folded the repeated bounds checking into a small helper and respun
> the patch as v2.
> 
> Thanks
> 
> v2:
> - fold the repeated Set Feature bounds checks into a helper
> - use the helper for all Set Feature write-attribute branches
> 
>  hw/cxl/cxl-mailbox-utils.c | 94 ++++++++++++++++++++++++------
>  tests/qtest/cxl-test.c     | 99 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 169 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
> index d8ba7e8625..4c7a083e4c 100644
> --- a/hw/cxl/cxl-mailbox-utils.c
> +++ b/hw/cxl/cxl-mailbox-utils.c
> @@ -1702,6 +1702,21 @@ static CXLRetCode cmd_features_get_feature(const struct cxl_cmd *cmd,
>      return CXL_MBOX_SUCCESS;
>  }
>  
> +static CXLRetCode cxl_set_feature_copy(void *write_attrs,
> +                                       size_t write_attrs_size,
> +                                       uint16_t offset,
> +                                       const void *payload,
> +                                       uint16_t bytes_to_copy)
> +{
> +    if ((uint32_t)offset + bytes_to_copy > write_attrs_size) {
> +        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> +    }
> +
> +    memcpy((uint8_t *)write_attrs + offset, payload, bytes_to_copy);
> +
> +    return CXL_MBOX_SUCCESS;
> +}

Ideally split the refactor part and existing usecases out as a precursor patch.
Then in a second patch add the missing checks.


> +
>  /* CXL r3.1 section 8.2.9.6.3: Set Feature (Opcode 0502h) */
>  static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>                                             uint8_t *payload_in,
> @@ -1713,6 +1728,7 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>      CXLSetFeatureInHeader *hdr = (void *)payload_in;
>      CXLSetFeatureInfo *set_feat_info;
>      uint16_t bytes_to_copy = 0;
> +    CXLRetCode ret;
>      uint8_t data_transfer_flag;
>      CXLType3Dev *ct3d;
>      uint16_t count;
> @@ -1760,13 +1776,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>              return CXL_MBOX_UNSUPPORTED;
>          }
>  
> -        if ((uint32_t)hdr->offset + bytes_to_copy >
> -            sizeof(ct3d->patrol_scrub_wr_attrs)) {
> -            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> -        }
> -        memcpy((uint8_t *)&ct3d->patrol_scrub_wr_attrs + hdr->offset,
> -               ps_write_attrs,
> -               bytes_to_copy);
> +        ret = cxl_set_feature_copy(&ct3d->patrol_scrub_wr_attrs,
> +                                   sizeof(ct3d->patrol_scrub_wr_attrs),
> +                                   hdr->offset, ps_write_attrs,
> +                                   bytes_to_copy);
> +        if (ret) {
> +            return ret;
> +        }
>          set_feat_info->data_size += bytes_to_copy;
>  
>          if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
> @@ -1787,13 +1803,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>              return CXL_MBOX_UNSUPPORTED;
>          }
>  
> -        if ((uint32_t)hdr->offset + bytes_to_copy >
> -            sizeof(ct3d->ecs_wr_attrs)) {
> -            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
> -        }
> -        memcpy((uint8_t *)&ct3d->ecs_wr_attrs + hdr->offset,
> -               ecs_write_attrs,
> -               bytes_to_copy);
> +        ret = cxl_set_feature_copy(&ct3d->ecs_wr_attrs,
> +                                   sizeof(ct3d->ecs_wr_attrs),
> +                                   hdr->offset, ecs_write_attrs,
> +                                   bytes_to_copy);
> +        if (ret) {
> +            return ret;
> +        }
>          set_feat_info->data_size += bytes_to_copy;
>  
>          if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
> @@ -1813,8 +1829,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>              return CXL_MBOX_UNSUPPORTED;
>          }
>  
> -        memcpy((uint8_t *)&ct3d->soft_ppr_wr_attrs + hdr->offset,
> -               sppr_write_attrs, bytes_to_copy);
> +        ret = cxl_set_feature_copy(&ct3d->soft_ppr_wr_attrs,
> +                                   sizeof(ct3d->soft_ppr_wr_attrs),
> +                                   hdr->offset, sppr_write_attrs,
> +                                   bytes_to_copy);
> +        if (ret) {
> +            return ret;
> +        }
>          set_feat_info->data_size += bytes_to_copy;
>  
>          if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
> @@ -1832,8 +1853,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>              return CXL_MBOX_UNSUPPORTED;
>          }
>  
> -        memcpy((uint8_t *)&ct3d->hard_ppr_wr_attrs + hdr->offset,
> -               hppr_write_attrs, bytes_to_copy);
> +        ret = cxl_set_feature_copy(&ct3d->hard_ppr_wr_attrs,
> +                                   sizeof(ct3d->hard_ppr_wr_attrs),
> +                                   hdr->offset, hppr_write_attrs,
> +                                   bytes_to_copy);
> +        if (ret) {
> +            return ret;
> +        }
>          set_feat_info->data_size += bytes_to_copy;
>  
>          if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
> @@ -1851,8 +1877,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>              return CXL_MBOX_UNSUPPORTED;
>          }
>  
> -        memcpy((uint8_t *)&ct3d->cacheline_sparing_wr_attrs + hdr->offset,
> -               mem_sparing_write_attrs, bytes_to_copy);
> +        ret = cxl_set_feature_copy(&ct3d->cacheline_sparing_wr_attrs,
> +                                   sizeof(ct3d->cacheline_sparing_wr_attrs),
> +                                   hdr->offset, mem_sparing_write_attrs,
> +                                   bytes_to_copy);
> +        if (ret) {
> +            return ret;
> +        }
>          set_feat_info->data_size += bytes_to_copy;
>  
>          if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
> @@ -1869,8 +1900,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>              return CXL_MBOX_UNSUPPORTED;
>          }
>  
> -        memcpy((uint8_t *)&ct3d->row_sparing_wr_attrs + hdr->offset,
> -               mem_sparing_write_attrs, bytes_to_copy);
> +        ret = cxl_set_feature_copy(&ct3d->row_sparing_wr_attrs,
> +                                   sizeof(ct3d->row_sparing_wr_attrs),
> +                                   hdr->offset, mem_sparing_write_attrs,
> +                                   bytes_to_copy);
> +        if (ret) {
> +            return ret;
> +        }
>          set_feat_info->data_size += bytes_to_copy;
>  
>          if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
> @@ -1887,8 +1923,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>              return CXL_MBOX_UNSUPPORTED;
>          }
>  
> -        memcpy((uint8_t *)&ct3d->bank_sparing_wr_attrs + hdr->offset,
> -               mem_sparing_write_attrs, bytes_to_copy);
> +        ret = cxl_set_feature_copy(&ct3d->bank_sparing_wr_attrs,
> +                                   sizeof(ct3d->bank_sparing_wr_attrs),
> +                                   hdr->offset, mem_sparing_write_attrs,
> +                                   bytes_to_copy);
> +        if (ret) {
> +            return ret;
> +        }
>          set_feat_info->data_size += bytes_to_copy;
>  
>          if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
> @@ -1905,8 +1946,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
>              return CXL_MBOX_UNSUPPORTED;
>          }
>  
> -        memcpy((uint8_t *)&ct3d->rank_sparing_wr_attrs + hdr->offset,
> -               mem_sparing_write_attrs, bytes_to_copy);
> +        ret = cxl_set_feature_copy(&ct3d->rank_sparing_wr_attrs,
> +                                   sizeof(ct3d->rank_sparing_wr_attrs),
> +                                   hdr->offset, mem_sparing_write_attrs,
> +                                   bytes_to_copy);
> +        if (ret) {
> +            return ret;
> +        }
>          set_feat_info->data_size += bytes_to_copy;
>  
>          if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
>              data_transfer_flag == CXL_SET_FEATURE_FLAG_FINISH_DATA_TRANSFER) {
> diff --git a/tests/qtest/cxl-test.c b/tests/qtest/cxl-test.c
> index 8fb7e58d4f..a9fcd98736 100644
> --- a/tests/qtest/cxl-test.c
> +++ b/tests/qtest/cxl-test.c

Test should be a 3rd patch.  We may not want to backport this one.
Great to have it though!

> @@ -7,6 +7,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "libqtest-single.h"
> +#include "hw/cxl/cxl_device.h"
>  
>  #define QEMU_PXB_CMD \
>      "-machine q35,cxl=on " \
> @@ -59,6 +60,12 @@
>      "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
>      "-device cxl-type3,bus=rp0,volatile-memdev=cxl-mem0,lsa=lsa0,id=mem0 "
>  
> +#define QEMU_T3D_DIRECT_PMEM \
> +    "-machine q35,cxl=on -nodefaults " \
> +    "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
> +    "-object memory-backend-file,id=lsa0,mem-path=%s,size=1M " \
> +    "-device cxl-type3,bus=pcie.0,persistent-memdev=cxl-mem0,lsa=lsa0,id=pmem0 "
> +
>  #define QEMU_2T3D \
>      "-object memory-backend-file,id=cxl-mem0,mem-path=%s,size=256M " \
>      "-object memory-backend-file,id=lsa0,mem-path=%s,size=256M " \
> @@ -81,6 +88,17 @@
>      "-object memory-backend-file,id=lsa3,mem-path=%s,size=256M " \
>      "-device cxl-type3,bus=rp3,persistent-memdev=cxl-mem3,lsa=lsa3,id=pmem3 "
>  
> +#define CXL_T3D_DEVFN 0x08
> +#define CXL_T3D_BAR2_ADDR 0x10000000ULL
Can we add some info on why that address?
> +
> +typedef struct QEMU_PACKED CXLSetFeatureInHeaderTest {
> +    uint8_t uuid[16];
> +    uint32_t flags;
> +    uint16_t offset;
> +    uint8_t version;
> +    uint8_t rsvd[9];
> +} CXLSetFeatureInHeaderTest;
> +
>  static void cxl_basic_hb(void)
>  {
>      qtest_start("-machine q35,cxl=on");
> @@ -118,6 +136,85 @@ static void cxl_2root_port(void)
>  }
>  
>  #ifdef CONFIG_POSIX
> +static uint32_t cxl_test_pci_config_addr(uint8_t devfn, uint8_t offset)
> +{
> +    return 0x80000000U | (devfn << 8) | offset;

Can we have a define for that enable bit?

Given this file includes both x86 and arm virt tests can we add
something to names of these functions to make it clear they are x86 only?

> +}
> +
> +static void cxl_test_t3d_enable_bar2(void)
> +{
> +    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x18));
> +    outl(0xcfc, CXL_T3D_BAR2_ADDR);
> +    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x1c));
> +    outl(0xcfc, 0);
> +    outl(0xcf8, cxl_test_pci_config_addr(CXL_T3D_DEVFN, 0x04));
> +    outl(0xcfc, 0x2);
Can we express that 0x2 in terms of the fields in the register?

> +}
> +
> +static uint64_t cxl_test_t3d_mailbox_base(void)
> +{
> +    return CXL_T3D_BAR2_ADDR + CXL_MAILBOX_REGISTERS_OFFSET;
> +}
> +
> +static uint64_t cxl_test_t3d_payload_base(void)
> +{
> +    return cxl_test_t3d_mailbox_base() + A_CXL_DEV_CMD_PAYLOAD;
> +}
> +
> +static void cxl_test_t3d_submit_set_feature(const void *payload, size_t len)
> +{
> +    memwrite(cxl_test_t3d_payload_base(), payload, len);
> +    writeq(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_CMD,
> +           ((uint64_t)len << 16) | (0x05 << 8) | 0x02);
> +    writel(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_CTRL, 1);
> +}
> +
> +static uint16_t cxl_test_t3d_mailbox_errno(void)
> +{
> +    return (readq(cxl_test_t3d_mailbox_base() + A_CXL_DEV_MAILBOX_STS) >>
> +            32) & 0xffff;
> +}
> +
> +static void cxl_test_fill_set_feature_header(CXLSetFeatureInHeaderTest *hdr,
> +                                             const uint8_t uuid[16],
> +                                             uint16_t offset,
> +                                             uint8_t version)
> +{
> +    memset(hdr, 0, sizeof(*hdr));
> +    memcpy(hdr->uuid, uuid, 16);
> +    hdr->offset = cpu_to_le16(offset);
> +    hdr->version = version;
> +}
> +
> +static void cxl_t3d_set_feature_rejects_oversized_rank_sparing(void)

Would be nice to expand the tests to cover more cases, ideally including one
that works!

> +{
> +    static const uint8_t rank_sparing_uuid[16] = {
> +        0x34, 0xdb, 0xaf, 0xf5, 0x05, 0x52, 0x42, 0x81,
> +        0x8f, 0x76, 0xda, 0x0b, 0x5e, 0x7a, 0x76, 0xa7,
> +    };
> +    g_autoptr(GString) cmdline = g_string_new(NULL);
> +    g_autofree const char *tmpfs = NULL;
> +    uint8_t payload[CXL_MAILBOX_MAX_PAYLOAD_SIZE] = { 0 };
> +    CXLSetFeatureInHeaderTest *hdr = (void *)payload;
> +
> +    tmpfs = g_dir_make_tmp("cxl-test-XXXXXX", NULL);
> +    g_string_printf(cmdline, QEMU_T3D_DIRECT_PMEM, tmpfs, tmpfs);
> +
> +    qtest_start(cmdline->str);
> +    cxl_test_t3d_enable_bar2();
> +
> +    cxl_test_fill_set_feature_header(hdr, rank_sparing_uuid, 0,
> +                                     CXL_MEMDEV_SPARING_SET_FEATURE_VERSION);
> +    memset(payload + sizeof(*hdr), 0x41,
> +           sizeof(payload) - sizeof(*hdr));

I think that fits on one line.

> +    cxl_test_t3d_submit_set_feature(payload, sizeof(payload));
> +    g_assert_cmphex(cxl_test_t3d_mailbox_errno(), ==,
> +                    CXL_MBOX_INVALID_PAYLOAD_LENGTH);
> +
> +    qtest_end();
> +    rmdir(tmpfs);
> +}

[PATCH v3 0/2] hw/cxl: bound Set Feature writes

[Jia Jia]

Hi Jonathan,

Thanks for the review.

This v3 splits the helper extraction from the actual bug fix. Patch 1
is a refactor only: it folds the existing patrol scrub and ECS bounds
checks into a helper, with no intended functional change.

Patch 2 uses that helper for the remaining Set Feature branches that
still copy into fixed-size write-attribute buffers without bounds
validation. That patch carries the Fixes tags.

I have left the qtest out of this backportable bugfix series for now.
I can send it separately after addressing the x86-only and magic-value
comments.

Changes since v2:

- split helper extraction from the bug fix
- add Fixes tags to the bug-fixing patch
- leave the qtest for a separate follow-up

Jia Jia (2):
  hw/cxl: factor Set Feature write bounds helper
  hw/cxl: validate PPR and sparing Set Feature writes

 hw/cxl/cxl-mailbox-utils.c | 94 ++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 24 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] hw/cxl: factor Set Feature write bounds helper

[Jia Jia]

c1c4d6b38b13 added the same offset + length validation twice in
cmd_features_set_feature(), once for patrol scrub and once for ECS.

Factor that logic into a small helper so later patches can reuse the
same check for the other Set Feature write-attribute branches.

No functional change intended.

Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
 hw/cxl/cxl-mailbox-utils.c | 40 ++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index d8ba7e8625..2e4cc5824d 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1702,6 +1702,21 @@ static CXLRetCode cmd_features_get_feature(const struct cxl_cmd *cmd,
     return CXL_MBOX_SUCCESS;
 }
 
+static CXLRetCode cxl_set_feature_copy(void *write_attrs,
+                                       size_t write_attrs_size,
+                                       uint16_t offset,
+                                       const void *payload,
+                                       uint16_t bytes_to_copy)
+{
+    if ((uint32_t)offset + bytes_to_copy > write_attrs_size) {
+        return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+    }
+
+    memcpy((uint8_t *)write_attrs + offset, payload, bytes_to_copy);
+
+    return CXL_MBOX_SUCCESS;
+}
+
 /* CXL r3.1 section 8.2.9.6.3: Set Feature (Opcode 0502h) */
 static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
                                            uint8_t *payload_in,
@@ -1713,6 +1728,7 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
     CXLSetFeatureInHeader *hdr = (void *)payload_in;
     CXLSetFeatureInfo *set_feat_info;
     uint16_t bytes_to_copy = 0;
+    CXLRetCode ret;
     uint8_t data_transfer_flag;
     CXLType3Dev *ct3d;
     uint16_t count;
@@ -1760,13 +1776,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        if ((uint32_t)hdr->offset + bytes_to_copy >
-            sizeof(ct3d->patrol_scrub_wr_attrs)) {
-            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+        ret = cxl_set_feature_copy(&ct3d->patrol_scrub_wr_attrs,
+                                   sizeof(ct3d->patrol_scrub_wr_attrs),
+                                   hdr->offset, ps_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
         }
-        memcpy((uint8_t *)&ct3d->patrol_scrub_wr_attrs + hdr->offset,
-               ps_write_attrs,
-               bytes_to_copy);
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1787,13 +1803,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        if ((uint32_t)hdr->offset + bytes_to_copy >
-            sizeof(ct3d->ecs_wr_attrs)) {
-            return CXL_MBOX_INVALID_PAYLOAD_LENGTH;
+        ret = cxl_set_feature_copy(&ct3d->ecs_wr_attrs,
+                                   sizeof(ct3d->ecs_wr_attrs),
+                                   hdr->offset, ecs_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
         }
-        memcpy((uint8_t *)&ct3d->ecs_wr_attrs + hdr->offset,
-               ecs_write_attrs,
-               bytes_to_copy);
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
-- 
2.34.1

[PATCH v3 2/2] hw/cxl: validate PPR and sparing Set Feature writes

[Jia Jia]

The Post Package Repair and memory sparing Set Feature branches still
copy mailbox payload data into fixed-size write-attribute objects
without checking that hdr->offset + bytes_to_copy stays within the
target buffer.

patrol scrub and ECS already reject oversized writes, but the later PPR
and sparing feature additions missed the same validation. A full
mailbox payload can therefore overrun the target write-attribute object,
for example in the rank sparing branch.

Use cxl_set_feature_copy() there as well and return
CXL_MBOX_INVALID_PAYLOAD_LENGTH for oversized requests.

Fixes: 5e5a86bab83 ("hw/cxl: Add support for Maintenance command and Post Package Repair (PPR)")
Fixes: da5cafdc4dd ("hw/cxl: Add emulation for memory sparing control feature")
Link: https://gitlab.com/qemu-project/qemu/-/work_items/3458
Cc: qemu-stable@nongnu.org
Signed-off-by: Jia Jia <physicalmtea@gmail.com>
---
 hw/cxl/cxl-mailbox-utils.c | 54 +++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 12 deletions(-)

diff --git a/hw/cxl/cxl-mailbox-utils.c b/hw/cxl/cxl-mailbox-utils.c
index 2e4cc5824d..4c7a083e4c 100644
--- a/hw/cxl/cxl-mailbox-utils.c
+++ b/hw/cxl/cxl-mailbox-utils.c
@@ -1829,8 +1829,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->soft_ppr_wr_attrs + hdr->offset,
-               sppr_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->soft_ppr_wr_attrs,
+                                   sizeof(ct3d->soft_ppr_wr_attrs),
+                                   hdr->offset, sppr_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1848,8 +1853,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->hard_ppr_wr_attrs + hdr->offset,
-               hppr_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->hard_ppr_wr_attrs,
+                                   sizeof(ct3d->hard_ppr_wr_attrs),
+                                   hdr->offset, hppr_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1867,8 +1877,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->cacheline_sparing_wr_attrs + hdr->offset,
-               mem_sparing_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->cacheline_sparing_wr_attrs,
+                                   sizeof(ct3d->cacheline_sparing_wr_attrs),
+                                   hdr->offset, mem_sparing_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1885,8 +1900,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->row_sparing_wr_attrs + hdr->offset,
-               mem_sparing_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->row_sparing_wr_attrs,
+                                   sizeof(ct3d->row_sparing_wr_attrs),
+                                   hdr->offset, mem_sparing_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1903,8 +1923,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->bank_sparing_wr_attrs + hdr->offset,
-               mem_sparing_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->bank_sparing_wr_attrs,
+                                   sizeof(ct3d->bank_sparing_wr_attrs),
+                                   hdr->offset, mem_sparing_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
@@ -1921,8 +1946,13 @@ static CXLRetCode cmd_features_set_feature(const struct cxl_cmd *cmd,
             return CXL_MBOX_UNSUPPORTED;
         }
 
-        memcpy((uint8_t *)&ct3d->rank_sparing_wr_attrs + hdr->offset,
-               mem_sparing_write_attrs, bytes_to_copy);
+        ret = cxl_set_feature_copy(&ct3d->rank_sparing_wr_attrs,
+                                   sizeof(ct3d->rank_sparing_wr_attrs),
+                                   hdr->offset, mem_sparing_write_attrs,
+                                   bytes_to_copy);
+        if (ret) {
+            return ret;
+        }
         set_feat_info->data_size += bytes_to_copy;
 
         if (data_transfer_flag == CXL_SET_FEATURE_FLAG_FULL_DATA_TRANSFER ||
-- 
2.34.1