[PATCH] cxl/features: Serialize multi-part Get/Set Feature transfers

[PATCH] cxl/features: Serialize multi-part Get/Set Feature transfers

[Dave Jiang]

A Get or Set Feature payload larger than the mailbox payload size is
split into several mailbox commands. mbox_mutex only serializes
individual mailbox commands and is dropped between iterations of these
loops. Nothing serializes the multi-part transfer as a whole.
cxl_get_feature() and cxl_set_feature() are reachable concurrently
from fwctl (per-fd RPCs run under a read-held registration lock) and
from the EDAC scrub/ECS/repair paths, so two transfers to the same
mailbox can interleave their parts and corrupt the device's transfer
context.

Add a per-mailbox feat_mutex and hold it across the whole transfer in
both functions. It nests outside mbox_mutex (which is taken inside
cxl_internal_send_cmd()), and is taken nowhere else, so no lock-ordering
inversion is introduced.

Link: https://sashiko.dev/#/patchset/20260702090849.47501-1-icheng@nvidia.com?part=1
Fixes: 5e5ac21f629d ("cxl/mbox: Add GET_FEATURE mailbox command")
Fixes: 14d502cc2718 ("cxl/mbox: Add SET_FEATURE mailbox command")
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
---
 drivers/cxl/core/features.c | 3 +++
 drivers/cxl/core/mbox.c     | 1 +
 include/cxl/mailbox.h       | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
index 85185af46b72..85d6bcdc360e 100644
--- a/drivers/cxl/core/features.c
+++ b/drivers/cxl/core/features.c
@@ -240,6 +240,8 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
 	size_out = min(feat_out_size, cxl_mbox->payload_size);
 	uuid_copy(&pi.uuid, feat_uuid);
 	pi.selection = selection;
+
+	guard(mutex)(&cxl_mbox->feat_mutex);
 	do {
 		data_to_rd_size = min(feat_out_size - data_rcvd_size,
 				      cxl_mbox->payload_size);
@@ -314,6 +316,7 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox,
 		data_in_size = cxl_mbox->payload_size - hdr_size;
 	}
 
+	guard(mutex)(&cxl_mbox->feat_mutex);
 	do {
 		int rc;
 
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
index 7c6c5b7450a5..0370ac39ec4a 100644
--- a/drivers/cxl/core/mbox.c
+++ b/drivers/cxl/core/mbox.c
@@ -1516,6 +1516,7 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
 
 	cxl_mbox->host = host;
 	mutex_init(&cxl_mbox->mbox_mutex);
+	mutex_init(&cxl_mbox->feat_mutex);
 	rcuwait_init(&cxl_mbox->mbox_wait);
 
 	return 0;
diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
index c4e99e2e3a9d..d008b9db07aa 100644
--- a/include/cxl/mailbox.h
+++ b/include/cxl/mailbox.h
@@ -50,6 +50,7 @@ struct cxl_mbox_cmd {
  * @payload_size: Size of space for payload
  *                (CXL 3.1 8.2.8.4.3 Mailbox Capabilities Register)
  * @mbox_mutex: mutex protects device mailbox and firmware
+ * @feat_mutex: serializes multi-part Get/Set Feature transfers
  * @mbox_wait: rcuwait for mailbox
  * @mbox_send: @dev specific transport for transmitting mailbox commands
  * @feat_cap: Features capability
@@ -60,6 +61,7 @@ struct cxl_mailbox {
 	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
 	size_t payload_size;
 	struct mutex mbox_mutex; /* lock to protect mailbox context */
+	struct mutex feat_mutex;
 	struct rcuwait mbox_wait;
 	int (*mbox_send)(struct cxl_mailbox *cxl_mbox, struct cxl_mbox_cmd *cmd);
 	enum cxl_features_capability feat_cap;

base-commit: 8cdeaa50eae8dad34885515f62559ee83e7e8dda
-- 
2.54.0

Re: [PATCH] cxl/features: Serialize multi-part Get/Set Feature transfers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Buffer overflow in `cxl_get_feature()` due to static `size_out` usage in multi-part transfers.
--

--- Patch [1]: [PATCH] cxl/features: Serialize multi-part Get/Set Feature transfers ---
commit a8495b40ab103b124442f90daa5b684383d16566
Author: Dave Jiang <dave.jiang@intel.com>

cxl/features: Serialize multi-part Get/Set Feature transfers

This patch introduces a per-mailbox feat_mutex to serialize multi-part
Get and Set Feature payload transfers. This ensures that concurrent accesses
from fwctl and EDAC paths cannot interleave parts and corrupt the device's
transfer context.

Link: https://sashiko.dev/#/patchset/20260702090849.47501-1-icheng@nvidia.com?part=1
Fixes: 5e5ac21f629d ("cxl/mbox: Add GET_FEATURE mailbox command")
Fixes: 14d502cc2718 ("cxl/mbox: Add SET_FEATURE mailbox command")

> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 85185af46b72d..85d6bcdc360ef 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -240,6 +240,8 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>  	size_out = min(feat_out_size, cxl_mbox->payload_size);
>  	uuid_copy(&pi.uuid, feat_uuid);
>  	pi.selection = selection;
> +
> +	guard(mutex)(&cxl_mbox->feat_mutex);
>  	do {
>  		data_to_rd_size = min(feat_out_size - data_rcvd_size,
>  				      cxl_mbox->payload_size);

[Severity: Critical]
This is a pre-existing issue, but does this static size_out assignment risk
a buffer overflow on the final iteration?

In cxl_get_feature(), mbox_cmd.size_out is assigned the initial size_out
value on every loop iteration, rather than the dynamically bounded
data_to_rd_size:

		mbox_cmd = (struct cxl_mbox_cmd) {
			.opcode = CXL_MBOX_OP_GET_FEATURE,
			.size_in = sizeof(pi),
			.payload_in = &pi,
			.size_out = size_out,
			.payload_out = feat_out + data_rcvd_size,
			.min_out = data_to_rd_size,
		};

If the hardware returns more bytes than requested on the final iteration,
could the transport use this larger size_out bound to copy past the end of
the feat_out buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709155841.1895915-1-dave.jiang@intel.com?part=1

Re: [PATCH] cxl/features: Serialize multi-part Get/Set Feature transfers

[Richard Cheng]

On Thu, Jul 09, 2026 at 08:58:41AM +0800, Dave Jiang wrote:
> A Get or Set Feature payload larger than the mailbox payload size is
> split into several mailbox commands. mbox_mutex only serializes
> individual mailbox commands and is dropped between iterations of these
> loops. Nothing serializes the multi-part transfer as a whole.
> cxl_get_feature() and cxl_set_feature() are reachable concurrently
> from fwctl (per-fd RPCs run under a read-held registration lock) and
> from the EDAC scrub/ECS/repair paths, so two transfers to the same
> mailbox can interleave their parts and corrupt the device's transfer
> context.
>

Hi Dave,

I have some opinion maybe worth discussing.

Get Feature does not have transfer state, it uses an offset and count.
For Set Feature, the device must reject a second transfer with Feature
Transfer in progress.

Maybe we should reword this as preventing concurrent Set failures and
preventing a multi-part Get from spanning a Set update? I don't think
"currupt the device's transfer context" is accutate.

> Add a per-mailbox feat_mutex and hold it across the whole transfer in
> both functions. It nests outside mbox_mutex (which is taken inside
> cxl_internal_send_cmd()), and is taken nowhere else, so no lock-ordering
> inversion is introduced.
> 
> Link: https://sashiko.dev/#/patchset/20260702090849.47501-1-icheng@nvidia.com?part=1
> Fixes: 5e5ac21f629d ("cxl/mbox: Add GET_FEATURE mailbox command")
> Fixes: 14d502cc2718 ("cxl/mbox: Add SET_FEATURE mailbox command")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  drivers/cxl/core/features.c | 3 +++
>  drivers/cxl/core/mbox.c     | 1 +
>  include/cxl/mailbox.h       | 2 ++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/drivers/cxl/core/features.c b/drivers/cxl/core/features.c
> index 85185af46b72..85d6bcdc360e 100644
> --- a/drivers/cxl/core/features.c
> +++ b/drivers/cxl/core/features.c
> @@ -240,6 +240,8 @@ size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>  	size_out = min(feat_out_size, cxl_mbox->payload_size);
>  	uuid_copy(&pi.uuid, feat_uuid);
>  	pi.selection = selection;
> +
> +	guard(mutex)(&cxl_mbox->feat_mutex);
>  	do {
>  		data_to_rd_size = min(feat_out_size - data_rcvd_size,
>  				      cxl_mbox->payload_size);
> @@ -314,6 +316,7 @@ int cxl_set_feature(struct cxl_mailbox *cxl_mbox,
>  		data_in_size = cxl_mbox->payload_size - hdr_size;
>  	}
>  
> +	guard(mutex)(&cxl_mbox->feat_mutex);

What about RAW command path ?
With CONFIG_CXL_MEM_RAW_COMMANDS=y, GET_FEATURE and SET_FEATURE can be
sent directly through mbox_send() without taking feat_mutex.

Or you want to keep the scope minimum so RAW path is out of scope ?

Otherwise the locking and ordering looks correct to me.

Best regards,
Richard Cheng.

>  	do {
>  		int rc;
>  
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 7c6c5b7450a5..0370ac39ec4a 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1516,6 +1516,7 @@ int cxl_mailbox_init(struct cxl_mailbox *cxl_mbox, struct device *host)
>  
>  	cxl_mbox->host = host;
>  	mutex_init(&cxl_mbox->mbox_mutex);
> +	mutex_init(&cxl_mbox->feat_mutex);
>  	rcuwait_init(&cxl_mbox->mbox_wait);
>  
>  	return 0;
> diff --git a/include/cxl/mailbox.h b/include/cxl/mailbox.h
> index c4e99e2e3a9d..d008b9db07aa 100644
> --- a/include/cxl/mailbox.h
> +++ b/include/cxl/mailbox.h
> @@ -50,6 +50,7 @@ struct cxl_mbox_cmd {
>   * @payload_size: Size of space for payload
>   *                (CXL 3.1 8.2.8.4.3 Mailbox Capabilities Register)
>   * @mbox_mutex: mutex protects device mailbox and firmware
> + * @feat_mutex: serializes multi-part Get/Set Feature transfers
>   * @mbox_wait: rcuwait for mailbox
>   * @mbox_send: @dev specific transport for transmitting mailbox commands
>   * @feat_cap: Features capability
> @@ -60,6 +61,7 @@ struct cxl_mailbox {
>  	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
>  	size_t payload_size;
>  	struct mutex mbox_mutex; /* lock to protect mailbox context */
> +	struct mutex feat_mutex;
>  	struct rcuwait mbox_wait;
>  	int (*mbox_send)(struct cxl_mailbox *cxl_mbox, struct cxl_mbox_cmd *cmd);
>  	enum cxl_features_capability feat_cap;
> 
> base-commit: 8cdeaa50eae8dad34885515f62559ee83e7e8dda
> -- 
> 2.54.0
> 
>