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(-)
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
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
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.0On 7/17/26 8:39 AM, Li Qiang wrote: > 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. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Fri, 2026-07-17 at 23:39 +0800, Li Qiang wrote:
> 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>
> ---
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
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.0On 7/17/26 8:39 AM, Li Qiang wrote: > 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; Reviewed-by: Bart Van Assche <bvanassche@acm.org>
On Fri, 2026-07-17 at 23:39 +0800, Li Qiang wrote:
> 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>
> ---
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
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.0The 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.0On Fri, 2026-07-17 at 23:39 +0800, Li Qiang wrote:
> 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
the two Sashiko findings are a separate story, this patch is ok to me.
Reviewed-by: Bean Huo <beanhuo@micron.com>
On Fri, 2026-07-17 at 23:39 +0800, Li Qiang wrote:
> 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>
> ---
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
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.0On Fri, 2026-07-17 at 23:39 +0800, Li Qiang wrote:
> 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);
> --
consider Sashiko's suggestion, it's better to add __packed on struct rpmb_frame
{}?
On 7/22/26 5:33 AM, Bean Huo wrote:
> consider Sashiko's suggestion, it's better to add __packed on struct rpmb_frame
> {}?
Do not add __packed to an entire data structure. Only use it on
data structure members that need it.
Thanks,
Bart.
On Wed, 22 Jul 2026 06:54:16 -0700
Bart Van Assche <bvanassche@acm.org> wrote:
> On 7/22/26 5:33 AM, Bean Huo wrote:
> > consider Sashiko's suggestion, it's better to add __packed on struct rpmb_frame
> > {}?
>
> Do not add __packed to an entire data structure. Only use it on
> data structure members that need it.
That depends...
If the structure itself can be misaligned in memory then the structure
needs to be packed.
OTOH if there is (for example) a 32bit field on a 16bit boundary then
just marking that field __packed will remove the pad.
The compiler can then use two 16bit instructions to access the data
rather than four 8bit ones.
David
>
> Thanks,
>
> Bart.
>
On Fri, 2026-07-17 at 23:39 +0800, Li Qiang wrote: > 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> > --- Reviewed-by: Peter Wang <peter.wang@mediatek.com>
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.0On 7/17/26 8:39 AM, Li Qiang wrote:
> 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>
On Fri, 2026-07-17 at 23:39 +0800, Li Qiang wrote:
> 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>
> ---
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
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.
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
© 2016 - 2026 Red Hat, Inc.