* [PATCH v2] hw/cxl: bound Set Feature writes [not found] <CAFEAcA_DnrvSCVY3f2q=3OnXt0+708BcwSJ=KhMn1t3sbbXQbg@mail.gmail.com> @ 2026-04-29 15:27 ` Jia Jia 2026-05-18 16:50 ` Jonathan Cameron 2026-05-28 5:55 ` [PATCH v3 0/2] " Jia Jia 0 siblings, 2 replies; 5+ messages in thread From: Jia Jia @ 2026-04-29 15:27 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, jic23, linux-cxl, farosas, lvivier, pbonzini 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] hw/cxl: bound Set Feature writes 2026-04-29 15:27 ` [PATCH v2] hw/cxl: bound Set Feature writes Jia Jia @ 2026-05-18 16:50 ` Jonathan Cameron 2026-05-28 5:55 ` [PATCH v3 0/2] " Jia Jia 1 sibling, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2026-05-18 16:50 UTC (permalink / raw) To: Jia Jia; +Cc: qemu-devel, peter.maydell, linux-cxl, farosas, lvivier, pbonzini 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); > +} ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 0/2] hw/cxl: bound Set Feature writes 2026-04-29 15:27 ` [PATCH v2] hw/cxl: bound Set Feature writes Jia Jia 2026-05-18 16:50 ` Jonathan Cameron @ 2026-05-28 5:55 ` Jia Jia 2026-05-28 5:55 ` [PATCH v3 1/2] hw/cxl: factor Set Feature write bounds helper Jia Jia 2026-05-28 5:55 ` [PATCH v3 2/2] hw/cxl: validate PPR and sparing Set Feature writes Jia Jia 1 sibling, 2 replies; 5+ messages in thread From: Jia Jia @ 2026-05-28 5:55 UTC (permalink / raw) To: qemu-devel Cc: Jonathan Cameron, Jonathan Cameron, Fan Ni, Fabiano Rosas, Laurent Vivier, Paolo Bonzini, linux-cxl 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v3 1/2] hw/cxl: factor Set Feature write bounds helper 2026-05-28 5:55 ` [PATCH v3 0/2] " Jia Jia @ 2026-05-28 5:55 ` Jia Jia 2026-05-28 5:55 ` [PATCH v3 2/2] hw/cxl: validate PPR and sparing Set Feature writes Jia Jia 1 sibling, 0 replies; 5+ messages in thread From: Jia Jia @ 2026-05-28 5:55 UTC (permalink / raw) To: qemu-devel Cc: Jonathan Cameron, Jonathan Cameron, Fan Ni, Fabiano Rosas, Laurent Vivier, Paolo Bonzini, linux-cxl 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v3 2/2] hw/cxl: validate PPR and sparing Set Feature writes 2026-05-28 5:55 ` [PATCH v3 0/2] " Jia Jia 2026-05-28 5:55 ` [PATCH v3 1/2] hw/cxl: factor Set Feature write bounds helper Jia Jia @ 2026-05-28 5:55 ` Jia Jia 1 sibling, 0 replies; 5+ messages in thread From: Jia Jia @ 2026-05-28 5:55 UTC (permalink / raw) To: qemu-devel Cc: Jonathan Cameron, Jonathan Cameron, Fan Ni, Fabiano Rosas, Laurent Vivier, Paolo Bonzini, linux-cxl, qemu-stable 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-05-28 5:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAFEAcA_DnrvSCVY3f2q=3OnXt0+708BcwSJ=KhMn1t3sbbXQbg@mail.gmail.com>
2026-04-29 15:27 ` [PATCH v2] hw/cxl: bound Set Feature writes Jia Jia
2026-05-18 16:50 ` Jonathan Cameron
2026-05-28 5:55 ` [PATCH v3 0/2] " Jia Jia
2026-05-28 5:55 ` [PATCH v3 1/2] hw/cxl: factor Set Feature write bounds helper Jia Jia
2026-05-28 5:55 ` [PATCH v3 2/2] hw/cxl: validate PPR and sparing Set Feature writes Jia Jia
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox