Re: [PATCH v2] hw/cxl: bound Set Feature writes
On Wed, 29 Apr 2026 23:27:50 +0800
Jia Jia 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
> ---
> 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),
> +
[PATCH v2] hw/cxl: bound Set Feature writes
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
---
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
