[PATCH 0/5] scsi: ufs: Fix descriptor parsing and invalid input handling

[PATCH 0/5] scsi: ufs: Fix descriptor parsing and invalid input handling

[liqiang]

From: Li Qiang <liqiang01@kylinos.cn>

This series fixes five independently triggerable UFS robustness issues.

The first patch validates string descriptor payload sizes and avoids raw
descriptor overreads. The remaining patches protect command completion and
MCQ cleanup, validate connected lane counts, validate RPMB frame sizes before
parsing, and retain a NUL terminator for debugfs input.

The series was built with x86_64 defconfig plus UFS, RPMB, and debugfs.

Li Qiang (5):
  scsi: ufs: core: Validate string descriptors
  scsi: ufs: Fix NULL dereferences after tag lookup
  scsi: ufs: core: Validate connected lane counts
  scsi: ufs: rpmb: Validate frame before parsing
  scsi: ufs: debugfs: Reserve space for a string terminator

 drivers/ufs/core/ufs-debugfs.c |  2 +-
 drivers/ufs/core/ufs-mcq.c     |  6 ++++--
 drivers/ufs/core/ufs-rpmb.c    |  8 ++++++--
 drivers/ufs/core/ufshcd.c      | 35 +++++++++++++++++++++++++---------
 4 files changed, 37 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH 1/5] scsi: ufs: core: Validate string descriptors

[liqiang]

From: Li Qiang <liqiang01@kylinos.cn>

The string descriptor length includes a two-byte header while the
UTF-16 payload starts after it. utf16s_to_utf8s() expects a count
of UTF-16 code units, not bytes. Passing the payload byte count can
make it read beyond the descriptor buffer.

Validate that the payload has an even byte count, pass a code-unit
count to the converter, and allocate sufficient UTF-8 output space.

The raw string buffer starts after the descriptor header but its size
is bLength. Copying bLength bytes from that pointer can read beyond
the response buffer.

Allocate a zeroed bLength-sized buffer and copy only the UTF-16
payload. This preserves the raw buffer size consumed by the RPMB
device-ID ABI while avoiding the overread.

Fixes: 4b828fe156a6 ("scsi: ufs: revamp string descriptor reading")
Fixes: d794b499f948 ("scsi: ufs: core: fix incorrect buffer duplication in ufshcd_read_string_desc()")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufshcd.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d3044a3089b5..3541da5b6f4d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3865,7 +3865,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
 {
 	struct uc_string_id *uc_str;
 	u8 *str;
-	int ret;
+	int ret, uc_len;
 
 	if (!buf)
 		return -EINVAL;
@@ -3890,11 +3890,19 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
 		goto out;
 	}
 
+	uc_len = uc_str->len - QUERY_DESC_HDR_SIZE;
+	if (uc_len % sizeof(*uc_str->uc)) {
+		dev_err(hba->dev, "String Desc has an odd UTF-16 payload length\n");
+		str = NULL;
+		ret = -EINVAL;
+		goto out;
+	}
+
 	if (fmt == SD_ASCII_STD) {
 		ssize_t ascii_len;
 		int i;
-		/* remove header and divide by 2 to move from UTF16 to UTF8 */
-		ascii_len = (uc_str->len - QUERY_DESC_HDR_SIZE) / 2 + 1;
+		/* Allow up to three UTF-8 bytes per UTF-16 code unit plus a NUL. */
+		ascii_len = uc_len / sizeof(*uc_str->uc) * 3 + 1;
 		str = kzalloc(ascii_len, GFP_KERNEL);
 		if (!str) {
 			ret = -ENOMEM;
@@ -3906,7 +3914,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
 		 * we need to convert to utf-8 so it can be displayed
 		 */
 		ret = utf16s_to_utf8s(uc_str->uc,
-				      uc_str->len - QUERY_DESC_HDR_SIZE,
+				      uc_len / sizeof(*uc_str->uc),
 				      UTF16_BIG_ENDIAN, str, ascii_len - 1);
 
 		/* replace non-printable or non-ASCII characters with spaces */
@@ -3916,11 +3924,17 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
 		str[ret++] = '\0';
 
 	} else {
-		str = kmemdup(uc_str->uc, uc_str->len, GFP_KERNEL);
+		/*
+		 * Keep the bLength-sized raw output for the RPMB device ID ABI.
+		 * The two bytes beyond the UTF-16 payload are explicitly zeroed
+		 * instead of being read past the descriptor buffer.
+		 */
+		str = kzalloc(uc_str->len, GFP_KERNEL);
 		if (!str) {
 			ret = -ENOMEM;
 			goto out;
 		}
+		memcpy(str, uc_str->uc, uc_len);
 		ret = uc_str->len;
 	}
 out:
-- 
2.43.0

[PATCH 2/5] scsi: ufs: Fix NULL dereferences after tag lookup

[liqiang]

From: Li Qiang <liqiang01@kylinos.cn>

ufshcd_tag_to_cmd() can return NULL when no command is associated
with a tag. The MCQ cleanup and completion paths dereferenced the
result before checking it.

Move private-command and request lookups after the NULL checks. Also
avoid dereferencing cqe while reporting an invalid tag because the
single-doorbell completion path passes a NULL CQE.

Fixes: 22089c218037 ("scsi: ufs: core: Optimize the hot path")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufs-mcq.c | 6 ++++--
 drivers/ufs/core/ufshcd.c  | 7 ++++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index 13b60a2d06db..15850b0658f9 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -555,8 +555,8 @@ static int ufshcd_mcq_sq_start(struct ufs_hba *hba, struct ufs_hw_queue *hwq)
 int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
 {
 	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, task_tag);
-	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
-	struct request *rq = scsi_cmd_to_rq(cmd);
+	struct ufshcd_lrb *lrbp;
+	struct request *rq;
 	struct ufs_hw_queue *hwq;
 	void __iomem *reg, *opr_sqd_base;
 	u32 nexus, id, val;
@@ -568,6 +568,8 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
 	if (!cmd)
 		return -EINVAL;
 
+	lrbp = scsi_cmd_priv(cmd);
+	rq = scsi_cmd_to_rq(cmd);
 	hwq = ufshcd_mcq_req_to_hwq(hba, rq);
 	if (!hwq)
 		return 0;
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3541da5b6f4d..f2a5fa624e74 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5857,13 +5857,14 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 			  struct cq_entry *cqe)
 {
 	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, task_tag);
-	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
+	struct ufshcd_lrb *lrbp;
 	enum utp_ocs ocs;
 
-	if (WARN_ONCE(!cmd, "cqe->command_desc_base_addr = %#llx\n",
-		      le64_to_cpu(cqe->command_desc_base_addr)))
+	if (WARN_ONCE(!cmd, "invalid completion tag %d, cqe->command_desc_base_addr = %#llx\n",
+		      task_tag, cqe ? le64_to_cpu(cqe->command_desc_base_addr) : 0ULL))
 		return;
 
+	lrbp = scsi_cmd_priv(cmd);
 	if (hba->monitor.enabled) {
 		lrbp->compl_time_stamp = ktime_get();
 		lrbp->compl_time_stamp_local_clock = local_clock();
-- 
2.43.0

[PATCH 3/5] scsi: ufs: core: Validate connected lane counts

[liqiang]

From: Li Qiang <liqiang01@kylinos.cn>

The connected lane count is used by TX equalization code to index
arrays sized by UFS_MAX_LANES. Reject zero and out-of-range RX or TX
lane counts before they can be propagated.

Fixes: 03e5d38e2f98 ("scsi: ufs: core: Add support for TX Equalization")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufshcd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index f2a5fa624e74..1319e1071025 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4729,7 +4729,9 @@ static int ufshcd_get_max_pwr_mode(struct ufs_hba *hba)
 	ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
 			&pwr_info->lane_tx);
 
-	if (!pwr_info->lane_rx || !pwr_info->lane_tx) {
+	if (!pwr_info->lane_rx || !pwr_info->lane_tx ||
+	    pwr_info->lane_rx > UFS_MAX_LANES ||
+	    pwr_info->lane_tx > UFS_MAX_LANES) {
 		dev_err(hba->dev, "%s: invalid connected lanes value. rx=%d, tx=%d\n",
 				__func__,
 				pwr_info->lane_rx,
-- 
2.43.0

[PATCH 4/5] scsi: ufs: rpmb: Validate frame before parsing

[liqiang]

From: Li Qiang <liqiang01@kylinos.cn>

The RPMB core only verifies that request and response buffers are
nonempty. This callback reads req_resp at the end of the first request
frame before validating the request length.

Require a complete frame before that access. Use unaligned accessors
because RPMB buffers are passed as u8 pointers and do not have an
alignment guarantee.

Fixes: b06b8c421485 ("scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufs-rpmb.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
index ffad049872b9..53f66b274aca 100644
--- a/drivers/ufs/core/ufs-rpmb.c
+++ b/drivers/ufs/core/ufs-rpmb.c
@@ -69,7 +69,11 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
 
 	hba = ufs_rpmb->hba;
 
-	req_type = be16_to_cpu(frm_out->req_resp);
+	/* req_resp is at the end of an RPMB frame. */
+	if (req_len < sizeof(*frm_out))
+		return -EINVAL;
+
+	req_type = get_unaligned_be16(&frm_out->req_resp);
 
 	switch (req_type) {
 	case RPMB_PROGRAM_KEY:
@@ -107,7 +111,7 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
 		struct rpmb_frame *frm_resp = (struct rpmb_frame *)resp;
 
 		memset(frm_resp, 0, sizeof(*frm_resp));
-		frm_resp->req_resp = cpu_to_be16(RPMB_RESULT_READ);
+		put_unaligned_be16(RPMB_RESULT_READ, &frm_resp->req_resp);
 		ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, true);
 		if (ret) {
 			dev_err(dev, "Result read request failed with ret=%d\n", ret);
-- 
2.43.0

[PATCH 5/5] scsi: ufs: debugfs: Reserve space for a string terminator

[liqiang]

From: Li Qiang <liqiang01@kylinos.cn>

ufs_saved_err_write() copies user input into a zero-initialized stack
buffer and passes it to kstrtoint(). A write that fills the entire
buffer overwrites its only terminator.

Reject an input whose length leaves no room for the trailing NUL.

Fixes: 7340faae9474 ("scsi: ufs: core: Add debugfs attributes for triggering the UFS EH")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufs-debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-debugfs.c b/drivers/ufs/core/ufs-debugfs.c
index e3dd81d6fe82..be527209540d 100644
--- a/drivers/ufs/core/ufs-debugfs.c
+++ b/drivers/ufs/core/ufs-debugfs.c
@@ -165,7 +165,7 @@ static ssize_t ufs_saved_err_write(struct file *file, const char __user *buf,
 	char val_str[16] = { };
 	int val, ret;
 
-	if (count > sizeof(val_str))
+	if (count >= sizeof(val_str))
 		return -EINVAL;
 	if (copy_from_user(val_str, buf, count))
 		return -EFAULT;
-- 
2.43.0

Re: [PATCH 4/5] scsi: ufs: rpmb: Validate frame before parsing

[sashiko-bot]

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

Pre-existing issues:
- [High] The UFS RPMB driver mismanages the lifecycle of `struct ufs_rpmb_dev`, resulting in Use-After-Free (UAF) vulnerabilities during teardown and unbind operations. This problem wasn't introduced by this patch, but it is a critical flaw in the driver's device management.
--

--- Patch [4]: [PATCH 4/5] scsi: ufs: rpmb: Validate frame before parsing ---
commit 52562de70e8a3a9ec694967f1ae0889763de9535
Author: Li Qiang <liqiang01@kylinos.cn>

scsi: ufs: rpmb: Validate frame before parsing
    
This commit validates the request length before accessing req_resp at the
end of the first request frame. It also uses unaligned accessors for safe
access to the RPMB buffers.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9a..53f66b274acac 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c

[ ... ]

> @@ -69,7 +69,11 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
>  
>  	hba = ufs_rpmb->hba;
>  
> -	req_type = be16_to_cpu(frm_out->req_resp);
> +	/* req_resp is at the end of an RPMB frame. */
> +	if (req_len < sizeof(*frm_out))
> +		return -EINVAL;
> +
> +	req_type = get_unaligned_be16(&frm_out->req_resp);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed a pre-existing issue with the lifecycle management of ufs_rpmb_dev.

Can this lead to a use-after-free during device unbind? The struct is
allocated using devm_kzalloc():

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_probe() {
    ...
    ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
    ...
}

But it embeds a reference-counted struct device. When the UFS host controller
unbinds, the devres cleanup executes and frees ufs_rpmb_dev. If a user-space
process holds a sysfs reference to the UFS RPMB device, the delayed
release callback will later access this freed memory:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_device_release() {
    ...
    rpmb_dev_unregister(ufs_rpmb->rdev);
    ...
}

Additionally, because rpmb_dev_unregister() is called asynchronously in the
.release callback rather than synchronously in ufs_rpmb_remove():

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_remove() {
    ...
    device_unregister(&ufs_rpmb->dev);
    ...
}

Could the RPMB core issue requests to a fully torn-down UFS host controller?

Should rpmb_dev_unregister() be moved to ufs_rpmb_remove(), and should
ufs_rpmb_dev be freed with kfree() inside ufs_rpmb_device_release() instead
of relying on devm?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784182493.git.liqiang01@kylinos.cn?part=4

Re: [PATCH 2/5] scsi: ufs: Fix NULL dereferences after tag lookup

[sashiko-bot]

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

Pre-existing issues:
- [High] If a malicious or malfunctioning UFS device sends a completion queue entry for a task tag that is not currently in-flight, `ufshcd_compl_one_cqe` will process it and release resources, leading to a double DMA unmap and power management underflow.
--

--- Patch [2]: [PATCH 2/5] scsi: ufs: Fix NULL dereferences after tag lookup ---
commit 070044fcdb23bd62a58cf9f22d642b3928421c36
Author: Li Qiang <liqiang01@kylinos.cn>

scsi: ufs: Fix NULL dereferences after tag lookup

This commit fixes a potential NULL pointer dereference in the MCQ cleanup
and completion paths by moving the private-command and request lookups
after the cmd NULL check. It also avoids dereferencing a NULL cqe when
reporting an invalid tag.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3541da5b6f4de..f2a5fa624e749 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5857,13 +5857,14 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>  			  struct cq_entry *cqe)
>  {
>  	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, task_tag);
> -	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
> +	struct ufshcd_lrb *lrbp;
>  	enum utp_ocs ocs;
>  
> -	if (WARN_ONCE(!cmd, "cqe->command_desc_base_addr = %#llx\n",
> -		      le64_to_cpu(cqe->command_desc_base_addr)))
> +	if (WARN_ONCE(!cmd, "invalid completion tag %d, cqe->command_desc_base_addr = %#llx\n",
> +		      task_tag, cqe ? le64_to_cpu(cqe->command_desc_base_addr) : 0ULL))
>  		return;
>  
> +	lrbp = scsi_cmd_priv(cmd);

[Severity: High]
This is a pre-existing issue, but can spurious completions from the device
cause double DMA unmaps and power management refcount underflows here?

Because ufshcd_tag_to_cmd() returns the memory allocated for the tag in the
block layer tagset (which is non-NULL even for inactive requests), the !cmd
check passes even if the tag isn't currently in-flight.

Later in ufshcd_compl_one_cqe(), ufshcd_release_scsi_cmd(hba, cmd) is called
unconditionally, bypassing the block layer's active request protections. If
a malicious or malfunctioning device sends a CQE for an inactive tag, it looks
like it could trigger a double DMA unmap and permanently break clock gating
by underflowing the PM refcounts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784182493.git.liqiang01@kylinos.cn?part=2

Re: [PATCH 0/5] scsi: ufs: Fix descriptor parsing and invalid input handling

[Bart Van Assche]

On 7/15/26 11:19 PM, liqiang wrote:
> The first patch validates string descriptor payload sizes and avoids raw
> descriptor overreads. The remaining patches protect command completion and
> MCQ cleanup, validate connected lane counts, validate RPMB frame sizes before
> parsing, and retain a NUL terminator for debugfs input.
Since this is the first time that you are contributing to the UFS 
driver: how have these issues been discovered? How have these issues
been tested? Is my understanding correct that these only have been
compile-tested?

Bart.

Re: [PATCH 1/5] scsi: ufs: core: Validate string descriptors

[Bart Van Assche]

On 7/15/26 11:19 PM, liqiang wrote:
> @@ -3916,11 +3924,17 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
>   		str[ret++] = '\0';
>   
>   	} else {
> -		str = kmemdup(uc_str->uc, uc_str->len, GFP_KERNEL);
> +		/*
> +		 * Keep the bLength-sized raw output for the RPMB device ID ABI.
> +		 * The two bytes beyond the UTF-16 payload are explicitly zeroed
> +		 * instead of being read past the descriptor buffer.
> +		 */
> +		str = kzalloc(uc_str->len, GFP_KERNEL);
>   		if (!str) {
>   			ret = -ENOMEM;
>   			goto out;
>   		}
> +		memcpy(str, uc_str->uc, uc_len);
>   		ret = uc_str->len;
>   	}

The above code can be simplified by using kasprintf(GFP_KERNEL, "%.*s",
...), isn't it?

Thanks,

Bart.

Re: [PATCH 2/5] scsi: ufs: Fix NULL dereferences after tag lookup

[Bart Van Assche]

On 7/15/26 11:19 PM, liqiang wrote:
> diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
> index 13b60a2d06db..15850b0658f9 100644
> --- a/drivers/ufs/core/ufs-mcq.c
> +++ b/drivers/ufs/core/ufs-mcq.c
> @@ -555,8 +555,8 @@ static int ufshcd_mcq_sq_start(struct ufs_hba *hba, struct ufs_hw_queue *hwq)
>   int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
>   {
>   	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, task_tag);
> -	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
> -	struct request *rq = scsi_cmd_to_rq(cmd);
> +	struct ufshcd_lrb *lrbp;
> +	struct request *rq;
>   	struct ufs_hw_queue *hwq;
>   	void __iomem *reg, *opr_sqd_base;
>   	u32 nexus, id, val;
> @@ -568,6 +568,8 @@ int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
>   	if (!cmd)
>   		return -EINVAL;
>   
> +	lrbp = scsi_cmd_priv(cmd);
> +	rq = scsi_cmd_to_rq(cmd);
>   	hwq = ufshcd_mcq_req_to_hwq(hba, rq);
>   	if (!hwq)
>   		return 0;

Please drop this change. Calling scsi_cmd_priv(NULL) or
scsi_cmd_to_rq(NULL) is safe if the pointers returned by these functions
are not used.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 3541da5b6f4d..f2a5fa624e74 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -5857,13 +5857,14 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
>   			  struct cq_entry *cqe)
>   {
>   	struct scsi_cmnd *cmd = ufshcd_tag_to_cmd(hba, task_tag);
> -	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
> +	struct ufshcd_lrb *lrbp;
>   	enum utp_ocs ocs;

Same comment here: calling scsi_cmd_priv(NULL) is safe if the returned
pointer is not used as is the case here.

>   
> -	if (WARN_ONCE(!cmd, "cqe->command_desc_base_addr = %#llx\n",
> -		      le64_to_cpu(cqe->command_desc_base_addr)))
> +	if (WARN_ONCE(!cmd, "invalid completion tag %d, cqe->command_desc_base_addr = %#llx\n",
> +		      task_tag, cqe ? le64_to_cpu(cqe->command_desc_base_addr) : 0ULL))
>   		return;

The above change looks good to me.

Thanks,

Bart.

Re: [PATCH 5/5] scsi: ufs: debugfs: Reserve space for a string terminator

[Bart Van Assche]

On 7/15/26 11:19 PM, liqiang wrote:
> From: Li Qiang <liqiang01@kylinos.cn>
> 
> ufs_saved_err_write() copies user input into a zero-initialized stack
> buffer and passes it to kstrtoint(). A write that fills the entire
> buffer overwrites its only terminator.
> 
> Reject an input whose length leaves no room for the trailing NUL.
> 
> Fixes: 7340faae9474 ("scsi: ufs: core: Add debugfs attributes for triggering the UFS EH")
> Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
> ---
>   drivers/ufs/core/ufs-debugfs.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufs-debugfs.c b/drivers/ufs/core/ufs-debugfs.c
> index e3dd81d6fe82..be527209540d 100644
> --- a/drivers/ufs/core/ufs-debugfs.c
> +++ b/drivers/ufs/core/ufs-debugfs.c
> @@ -165,7 +165,7 @@ static ssize_t ufs_saved_err_write(struct file *file, const char __user *buf,
>   	char val_str[16] = { };
>   	int val, ret;
>   
> -	if (count > sizeof(val_str))
> +	if (count >= sizeof(val_str))
>   		return -EINVAL;
>   	if (copy_from_user(val_str, buf, count))
>   		return -EFAULT;

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 4/5] scsi: ufs: rpmb: Validate frame before parsing

[Bart Van Assche]

On 7/15/26 11:19 PM, liqiang wrote:
> From: Li Qiang <liqiang01@kylinos.cn>
> 
> The RPMB core only verifies that request and response buffers are
> nonempty. This callback reads req_resp at the end of the first request
> frame before validating the request length.
> 
> Require a complete frame before that access. Use unaligned accessors
> because RPMB buffers are passed as u8 pointers and do not have an
> alignment guarantee.
> 
> Fixes: b06b8c421485 ("scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices")
> Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
> ---
>   drivers/ufs/core/ufs-rpmb.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9..53f66b274aca 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -69,7 +69,11 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
>   
>   	hba = ufs_rpmb->hba;
>   
> -	req_type = be16_to_cpu(frm_out->req_resp);
> +	/* req_resp is at the end of an RPMB frame. */
> +	if (req_len < sizeof(*frm_out))
> +		return -EINVAL;
> +
> +	req_type = get_unaligned_be16(&frm_out->req_resp);
>   
>   	switch (req_type) {
>   	case RPMB_PROGRAM_KEY:
> @@ -107,7 +111,7 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
>   		struct rpmb_frame *frm_resp = (struct rpmb_frame *)resp;
>   
>   		memset(frm_resp, 0, sizeof(*frm_resp));
> -		frm_resp->req_resp = cpu_to_be16(RPMB_RESULT_READ);
> +		put_unaligned_be16(RPMB_RESULT_READ, &frm_resp->req_resp);
>   		ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, true);
>   		if (ret) {
>   			dev_err(dev, "Result read request failed with ret=%d\n", ret);

Please split this patch into two patches because the two above changes 
are independent of each other.

Thanks,

Bart.

Re: [PATCH 0/5] scsi: ufs: Fix descriptor parsing and invalid input handling

[Li Qiang]

Hi Bart,

These issues were discovered with the assistance of an AI-based static code analysis tool. After manual review, I confirmed that they are real issues, so I submitted patches to fix them.

I have not tested them on physical hardware. Since these patches do not change the main functionality and only add security hardening, I think compile testing is sufficient.

Thanks,
Li Qiang

Re: [PATCH 1/5] scsi: ufs: core: Validate string descriptors

[Li Qiang]

Hi Bart,

I don't think kasprintf() is suitable here. This path must preserve the raw UTF-16 bytes and return a buffer of uc_str->len bytes for the RPMB device-ID ABI; %.*s treats the data as a C string and would not preserve embedded NUL bytes or the required two-byte padding.

Thanks,
Li Qiang

[PATCH v2 0/6] scsi: ufs: Fix descriptor parsing and invalid input handling

[Li Qiang]

The first patch validates string descriptor payload sizes and avoids raw
descriptor overreads. The remaining patches protect invalid completion
diagnostics, validate connected lane counts, validate RPMB frame sizes,
use unaligned RPMB frame accesses, and retain a NUL terminator for
debugfs input.

Changes in v2:
- Reworked patch 2 as requested: drop changes to the safe
  scsi_cmd_priv(NULL) and scsi_cmd_to_rq(NULL) calls and retain only the
  invalid-CQE diagnostic fix.
- Split the previous RPMB patch into a request-length validation patch
  and an independent unaligned-access patch.

Li Qiang (6):
  scsi: ufs: core: Validate string descriptors
  scsi: ufs: Avoid NULL CQE dereference when reporting invalid tags
  scsi: ufs: core: Validate connected lane counts
  scsi: ufs: rpmb: Validate request frame length before parsing
  scsi: ufs: rpmb: Use unaligned accessors for RPMB frames
  scsi: ufs: debugfs: Reserve space for a string terminator

 drivers/ufs/core/ufs-debugfs.c |  2 +-
 drivers/ufs/core/ufs-rpmb.c    |  8 ++++++--
 drivers/ufs/core/ufshcd.c      | 32 ++++++++++++++++++++++++--------
 3 files changed, 31 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] scsi: ufs: core: Validate string descriptors

[Li Qiang]

The string descriptor length includes a two-byte header while the
UTF-16 payload starts after it. utf16s_to_utf8s() expects a count
of UTF-16 code units, not bytes. Passing the payload byte count can
make it read beyond the descriptor buffer.

Validate that the payload has an even byte count, pass a code-unit
count to the converter, and allocate sufficient UTF-8 output space.

The raw string buffer starts after the descriptor header but its size
is bLength. Copying bLength bytes from that pointer can read beyond
the response buffer.

Allocate a zeroed bLength-sized buffer and copy only the UTF-16
payload. This preserves the raw buffer size consumed by the RPMB
device-ID ABI while avoiding the overread.

Fixes: 4b828fe156a6 ("scsi: ufs: revamp string descriptor reading")
Fixes: d794b499f948 ("scsi: ufs: core: fix incorrect buffer duplication in ufshcd_read_string_desc()")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufshcd.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d3044a3089b5..3541da5b6f4d 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3865,7 +3865,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
 {
 	struct uc_string_id *uc_str;
 	u8 *str;
-	int ret;
+	int ret, uc_len;
 
 	if (!buf)
 		return -EINVAL;
@@ -3890,11 +3890,19 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
 		goto out;
 	}
 
+	uc_len = uc_str->len - QUERY_DESC_HDR_SIZE;
+	if (uc_len % sizeof(*uc_str->uc)) {
+		dev_err(hba->dev, "String Desc has an odd UTF-16 payload length\n");
+		str = NULL;
+		ret = -EINVAL;
+		goto out;
+	}
+
 	if (fmt == SD_ASCII_STD) {
 		ssize_t ascii_len;
 		int i;
-		/* remove header and divide by 2 to move from UTF16 to UTF8 */
-		ascii_len = (uc_str->len - QUERY_DESC_HDR_SIZE) / 2 + 1;
+		/* Allow up to three UTF-8 bytes per UTF-16 code unit plus a NUL. */
+		ascii_len = uc_len / sizeof(*uc_str->uc) * 3 + 1;
 		str = kzalloc(ascii_len, GFP_KERNEL);
 		if (!str) {
 			ret = -ENOMEM;
@@ -3906,7 +3914,7 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
 		 * we need to convert to utf-8 so it can be displayed
 		 */
 		ret = utf16s_to_utf8s(uc_str->uc,
-				      uc_str->len - QUERY_DESC_HDR_SIZE,
+				      uc_len / sizeof(*uc_str->uc),
 				      UTF16_BIG_ENDIAN, str, ascii_len - 1);
 
 		/* replace non-printable or non-ASCII characters with spaces */
@@ -3916,11 +3924,17 @@ int ufshcd_read_string_desc(struct ufs_hba *hba, u8 desc_index, u8 **buf, enum u
 		str[ret++] = '\0';
 
 	} else {
-		str = kmemdup(uc_str->uc, uc_str->len, GFP_KERNEL);
+		/*
+		 * Keep the bLength-sized raw output for the RPMB device ID ABI.
+		 * The two bytes beyond the UTF-16 payload are explicitly zeroed
+		 * instead of being read past the descriptor buffer.
+		 */
+		str = kzalloc(uc_str->len, GFP_KERNEL);
 		if (!str) {
 			ret = -ENOMEM;
 			goto out;
 		}
+		memcpy(str, uc_str->uc, uc_len);
 		ret = uc_str->len;
 	}
 out:
-- 
2.43.0

[PATCH v2 2/6] scsi: ufs: Avoid NULL CQE dereference when reporting invalid tags

[Li Qiang]

The single-doorbell completion path can call ufshcd_compl_one_cqe() with

a NULL CQE. If no command is associated with the completion tag, the

warning message dereferences the CQE while reporting the error.

Avoid that dereference and include the invalid tag in the warning.

Fixes: 22089c218037 ("scsi: ufs: core: Optimize the hot path")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufshcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 3541da5b6f4d..d1b989abc9e2 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -5860,8 +5860,8 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 	struct ufshcd_lrb *lrbp = scsi_cmd_priv(cmd);
 	enum utp_ocs ocs;
 
-	if (WARN_ONCE(!cmd, "cqe->command_desc_base_addr = %#llx\n",
-		      le64_to_cpu(cqe->command_desc_base_addr)))
+	if (WARN_ONCE(!cmd, "invalid completion tag %d, cqe->command_desc_base_addr = %#llx\n",
+		      task_tag, cqe ? le64_to_cpu(cqe->command_desc_base_addr) : 0ULL))
 		return;
 
 	if (hba->monitor.enabled) {
-- 
2.43.0

[PATCH v2 3/6] scsi: ufs: core: Validate connected lane counts

[Li Qiang]

The connected lane count is used by TX equalization code to index
arrays sized by UFS_MAX_LANES. Reject zero and out-of-range RX or TX
lane counts before they can be propagated.

Fixes: 03e5d38e2f98 ("scsi: ufs: core: Add support for TX Equalization")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufshcd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index d1b989abc9e2..bbadddcd661a 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4729,7 +4729,9 @@ static int ufshcd_get_max_pwr_mode(struct ufs_hba *hba)
 	ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
 			&pwr_info->lane_tx);
 
-	if (!pwr_info->lane_rx || !pwr_info->lane_tx) {
+	if (!pwr_info->lane_rx || !pwr_info->lane_tx ||
+	    pwr_info->lane_rx > UFS_MAX_LANES ||
+	    pwr_info->lane_tx > UFS_MAX_LANES) {
 		dev_err(hba->dev, "%s: invalid connected lanes value. rx=%d, tx=%d\n",
 				__func__,
 				pwr_info->lane_rx,
-- 
2.43.0

[PATCH v2 4/6] scsi: ufs: rpmb: Validate request frame length before parsing

[Li Qiang]

The RPMB core only verifies that request and response buffers are

nonempty. This callback reads req_resp at the end of the first request

frame before validating the request length.

Require a complete frame before that access.

Fixes: b06b8c421485 ("scsi: ufs: core: Add OP-TEE based RPMB driver for UFS devices")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufs-rpmb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
index ffad049872b9..431f85b36876 100644
--- a/drivers/ufs/core/ufs-rpmb.c
+++ b/drivers/ufs/core/ufs-rpmb.c
@@ -69,6 +69,10 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
 
 	hba = ufs_rpmb->hba;
 
+	/* req_resp is at the end of an RPMB frame. */
+	if (req_len < sizeof(*frm_out))
+		return -EINVAL;
+
 	req_type = be16_to_cpu(frm_out->req_resp);
 
 	switch (req_type) {
-- 
2.43.0

[PATCH v2 5/6] scsi: ufs: rpmb: Use unaligned accessors for RPMB frames

[Li Qiang]

RPMB frame buffers are passed as u8 pointers and do not have an

alignment guarantee. Use unaligned accessors for the req_resp field.

Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufs-rpmb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
index 431f85b36876..53f66b274aca 100644
--- a/drivers/ufs/core/ufs-rpmb.c
+++ b/drivers/ufs/core/ufs-rpmb.c
@@ -73,7 +73,7 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
 	if (req_len < sizeof(*frm_out))
 		return -EINVAL;
 
-	req_type = be16_to_cpu(frm_out->req_resp);
+	req_type = get_unaligned_be16(&frm_out->req_resp);
 
 	switch (req_type) {
 	case RPMB_PROGRAM_KEY:
@@ -111,7 +111,7 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
 		struct rpmb_frame *frm_resp = (struct rpmb_frame *)resp;
 
 		memset(frm_resp, 0, sizeof(*frm_resp));
-		frm_resp->req_resp = cpu_to_be16(RPMB_RESULT_READ);
+		put_unaligned_be16(RPMB_RESULT_READ, &frm_resp->req_resp);
 		ret = ufs_sec_submit(hba, protocol_id, resp, resp_len, true);
 		if (ret) {
 			dev_err(dev, "Result read request failed with ret=%d\n", ret);
-- 
2.43.0

[PATCH v2 6/6] scsi: ufs: debugfs: Reserve space for a string terminator

[Li Qiang]

ufs_saved_err_write() copies user input into a zero-initialized stack
buffer and passes it to kstrtoint(). A write that fills the entire
buffer overwrites its only terminator.

Reject an input whose length leaves no room for the trailing NUL.

Fixes: 7340faae9474 ("scsi: ufs: core: Add debugfs attributes for triggering the UFS EH")
Signed-off-by: Li Qiang <liqiang01@kylinos.cn>
---
 drivers/ufs/core/ufs-debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufs-debugfs.c b/drivers/ufs/core/ufs-debugfs.c
index e3dd81d6fe82..be527209540d 100644
--- a/drivers/ufs/core/ufs-debugfs.c
+++ b/drivers/ufs/core/ufs-debugfs.c
@@ -165,7 +165,7 @@ static ssize_t ufs_saved_err_write(struct file *file, const char __user *buf,
 	char val_str[16] = { };
 	int val, ret;
 
-	if (count > sizeof(val_str))
+	if (count >= sizeof(val_str))
 		return -EINVAL;
 	if (copy_from_user(val_str, buf, count))
 		return -EFAULT;
-- 
2.43.0

Re: [PATCH v2 3/6] scsi: ufs: core: Validate connected lane counts

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds check on untrusted `tx_lanes` value read from the device in `ufshcd_disable_tx_lcc` could lead to an excessive loop and kernel hang.
--

--- Patch [3]: [PATCH v2 3/6] scsi: ufs: core: Validate connected lane counts ---
commit a03fd44c15e264d545e15508efe3bce7ec4082a8
Author: Li Qiang <liqiang01@kylinos.cn>

scsi: ufs: core: Validate connected lane counts

This commit updates ufshcd_get_max_pwr_mode() to validate that RX and TX
lane counts read from the device do not exceed UFS_MAX_LANES. It ensures
these counts are bounded before they are propagated.

> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index d1b989abc9e2d..bbadddcd661a0 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4729,7 +4729,9 @@ static int ufshcd_get_max_pwr_mode(struct ufs_hba *hba)
>  	ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
>  			&pwr_info->lane_tx);
>  
> -	if (!pwr_info->lane_rx || !pwr_info->lane_tx) {
> +	if (!pwr_info->lane_rx || !pwr_info->lane_tx ||
> +	    pwr_info->lane_rx > UFS_MAX_LANES ||
> +	    pwr_info->lane_tx > UFS_MAX_LANES) {
>  		dev_err(hba->dev, "%s: invalid connected lanes value. rx=%d, tx=%d\n",
>  				__func__,
>  				pwr_info->lane_rx,

[Severity: High]
This isn't a bug introduced by this patch, but since we are adding bounds
checking for PA_CONNECTEDTXDATALANES here, is it possible we need similar
validation in ufshcd_disable_tx_lcc()?

When ufshcd_disable_tx_lcc() reads the connected TX lane count, it uses the
value directly as a loop boundary:

drivers/ufs/core/ufshcd.c:ufshcd_disable_tx_lcc() {
	...
	if (!peer)
		ufshcd_dme_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
			       &tx_lanes);
	else
		ufshcd_dme_peer_get(hba, UIC_ARG_MIB(PA_CONNECTEDTXDATALANES),
				    &tx_lanes);
	for (i = 0; i < tx_lanes; i++) {
	...
}

If a malicious endpoint or UniPro link error causes the read to return an
abnormally large integer, the code could enter an unvalidated loop.

If the endpoint continuously returns success to the synchronous
ufshcd_dme_peer_set() commands inside the loop, could this force the kernel
to execute an excessive number of slow hardware commands?

This might potentially block the probe or error handler thread and result
in a kernel hang.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717153914.26321-1-liqiang01@kylinos.cn?part=3

Re: [PATCH v2 4/6] scsi: ufs: rpmb: Validate request frame length before parsing

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing IS_ERR() check for tee_shm_get_va() return values in OP-TEE RPMB RPC handler.
- [High] Circular refcount and incorrect use of devm_kzalloc() for a dynamically registered struct device lead to a memory leak and severe Use-After-Free when UFS is unbound.
--

--- Patch [4]: [PATCH v2 4/6] scsi: ufs: rpmb: Validate request frame length before parsing ---
commit 1f68e5566b9d9ef98707ec46f344522aaa25cb6b
Author: Li Qiang <liqiang01@kylinos.cn>

scsi: ufs: rpmb: Validate request frame length before parsing

The RPMB core only verifies that request and response buffers are nonempty.
This commit adds a check to ensure a complete RPMB request frame is provided
before reading req_resp at the end of the frame.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index ffad049872b9a..431f85b368760 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -69,6 +69,10 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
>  
>  	hba = ufs_rpmb->hba;

[Severity: High]
This is a pre-existing issue, but does this lead to a use-after-free when the
UFS host is unbound?

Because ufs_rpmb was allocated with devm_kzalloc(), it is automatically freed
on unbind. However, ufs_rpmb_remove() unregisters ufs_rpmb->dev but fails to
unregister the child rdev. Since rdev->dev implicitly holds a parent reference
to ufs_rpmb->dev, ufs_rpmb->dev's refcount never drops to zero, and
ufs_rpmb_device_release() is never invoked to call rpmb_dev_unregister().

If a subsequent RPMB request routes a frame, will it extract and dereference
the freed ufs_rpmb object here?

> +	/* req_resp is at the end of an RPMB frame. */
> +	if (req_len < sizeof(*frm_out))
> +		return -EINVAL;
> +
>  	req_type = be16_to_cpu(frm_out->req_resp);

[Severity: High]
This isn't a bug introduced by this patch, but can we dereference an error
pointer here?

If drivers/tee/optee/rpc.c:handle_rpc_func_rpmb_frames() receives an
out-of-bounds offset from the Secure World, tee_shm_get_va() returns
ERR_PTR(-EINVAL). Because there is no IS_ERR() check, this error pointer is
passed as 'req' to rpmb_route_frames() and eventually reaches here as frm_out.

Will accessing frm_out->req_resp cause a panic if the Secure World sends an
invalid offset?

>  
>  	switch (req_type) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717153914.26321-1-liqiang01@kylinos.cn?part=4

Re: [PATCH v2 5/6] scsi: ufs: rpmb: Use unaligned accessors for RPMB frames

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but while the patch correctly fixes unaligned memory accesses in the UFS RPMB backend, identical unsafe accesses remain unpatched in the MMC RPMB backend. Additionally, a structural fix using `__packed` on `struct rpmb_frame` is missing, which would safely handle unaligned access for all backends globally.
--

--- Patch [5]: [PATCH v2 5/6] scsi: ufs: rpmb: Use unaligned accessors for RPMB frames ---
commit 521f1f28960728ee9f0014bd2898bd36bb94fa39
Author: Li Qiang <liqiang01@kylinos.cn>

scsi: ufs: rpmb: Use unaligned accessors for RPMB frames

RPMB frame buffers are passed as u8 pointers and do not have an
alignment guarantee. This commit uses unaligned accessors for the req_resp
field to prevent alignment fault traps.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index 431f85b368760..53f66b274acac 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c
> @@ -73,7 +73,7 @@ static int ufs_rpmb_route_frames(struct device *dev, u8 *req, unsigned int req_l
>  	if (req_len < sizeof(*frm_out))
>  		return -EINVAL;
>  
> -	req_type = be16_to_cpu(frm_out->req_resp);
> +	req_type = get_unaligned_be16(&frm_out->req_resp);

[Severity: High]
This is a pre-existing issue, but while this correctly fixes the unaligned
memory access in the UFS RPMB backend, does it make sense to fix this globally
by adding the __packed attribute to struct rpmb_frame in include/linux/rpmb.h?

Currently, identical unsafe accesses remain unpatched in the MMC RPMB backend,
such as in mmc_route_rpmb_frames() in drivers/mmc/core/block.c:

drivers/mmc/core/block.c:mmc_route_rpmb_frames() {
	...
	req_type = be16_to_cpu(frm->req_resp);
	...
	resp_frm->req_resp = cpu_to_be16(RPMB_RESULT_READ);
	...
}

An RPC call from the TEE secure world (e.g., OP-TEE) passing an unaligned
shm_offs to the kernel for an RPMB routing request
(handle_rpc_func_rpmb_frames) can cause tee_shm_get_va() to return an
unaligned u8 * pointer.

The MMC driver casts this to struct rpmb_frame * and dereferences req_resp,
which can trigger an alignment fault trap and kernel panic on architectures
that do not support unaligned memory access.

Would adding __packed to struct rpmb_frame instruct the compiler to safely
generate unaligned accesses for all members globally, avoiding brittle
driver-by-driver accessor fixes?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717153914.26321-1-liqiang01@kylinos.cn?part=5