[PATCH 0/3] Log correct rpmb unit descriptor size

[PATCH 0/3] Log correct rpmb unit descriptor size

[Avri Altman]

For the rpmb unit descriptor, if the field offset is larger than 0x23,
it may trigger a stack corruption because a) we do not log properly the
rpmb unit descriptor size,  and b) ufs_is_valid_unit_desc_lun() test for
specific wb offset case, and does not verify that the requested field
does not exceed the descriptor size.

Fix both issues.

Reported-by: Bart Van Assche <bvanassche@google.com>

Avri Altman (3):
  scsi: ufs: Remove redundant define
  scsi: ufs: Map the correct size to the rpmb unit descriptor
  scsi: ufs: Generalize ufs_is_valid_unit_desc_lun()

 drivers/scsi/ufs/ufs-sysfs.c |  2 +-
 drivers/scsi/ufs/ufs.h       | 21 +--------------------
 drivers/scsi/ufs/ufs_bsg.c   |  3 ++-
 drivers/scsi/ufs/ufshcd.c    | 19 ++++++++++++-------
 drivers/scsi/ufs/ufshcd.h    | 27 ++++++++++++++++++++++++++-
 5 files changed, 42 insertions(+), 30 deletions(-)

-- 
2.17.1

[PATCH 1/3] scsi: ufs: Remove redundant define

[Avri Altman]

UFS_UPIU_RPMB_WLUN already describe the rpmb wlun index.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufs.h    | 1 -
 drivers/scsi/ufs/ufshcd.c | 3 ++-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index cb80b9670bfe..579cf6f9e7a1 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -38,7 +38,6 @@
 #define UFS_UPIU_MAX_UNIT_NUM_ID	0x7F
 #define UFS_MAX_LUNS		(SCSI_W_LUN_BASE + UFS_UPIU_MAX_UNIT_NUM_ID)
 #define UFS_UPIU_WLUN_ID	(1 << 7)
-#define UFS_RPMB_UNIT		0xC4
 
 /* WriteBooster buffer is available only for the logical unit from 0 to 7 */
 #define UFS_UPIU_MAX_WB_LUN_ID	8
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 064a44e628d6..74ccfd2b80ce 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3334,7 +3334,8 @@ static void ufshcd_update_desc_length(struct ufs_hba *hba,
 				      unsigned char desc_len)
 {
 	if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
-	    desc_id != QUERY_DESC_IDN_STRING && desc_index != UFS_RPMB_UNIT)
+	    desc_id != QUERY_DESC_IDN_STRING &&
+	    desc_index != UFS_UPIU_RPMB_WLUN)
 		/* For UFS 3.1, the normal unit descriptor is 10 bytes larger
 		 * than the RPMB unit, however, both descriptors share the same
 		 * desc_idn, to cover both unit descriptors with one length, we
-- 
2.17.1

[PATCH 2/3] scsi: ufs: Map the correct size to the rpmb unit descriptor

[Avri Altman]

Each lun is designated by its unit descriptor. All regular luns share
the same unit descriptor size. The rpmb unit descriptor size, however,
is different.

Log the correct size for the rpmb unit descriptor in an unused
descriptor id number.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufs.h     |  1 +
 drivers/scsi/ufs/ufs_bsg.c |  3 ++-
 drivers/scsi/ufs/ufshcd.c  | 18 +++++++++++-------
 drivers/scsi/ufs/ufshcd.h  |  2 +-
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 579cf6f9e7a1..d0be8d4c8091 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -167,6 +167,7 @@ enum desc_idn {
 	QUERY_DESC_IDN_GEOMETRY		= 0x7,
 	QUERY_DESC_IDN_POWER		= 0x8,
 	QUERY_DESC_IDN_HEALTH           = 0x9,
+	QUERY_DESC_IDN_UNIT_RPMB	= 0xA,
 	QUERY_DESC_IDN_MAX,
 };
 
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 39bf204c6ec3..fcb46c882f1c 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -11,11 +11,12 @@ static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba, int *desc_len,
 {
 	int desc_size = be16_to_cpu(qr->length);
 	int desc_id = qr->idn;
+	int desc_index = qr->index;
 
 	if (desc_size <= 0)
 		return -EINVAL;
 
-	ufshcd_map_desc_id_to_length(hba, desc_id, desc_len);
+	ufshcd_map_desc_id_to_length(hba, desc_id, desc_index, desc_len);
 	if (!*desc_len)
 		return -EINVAL;
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 74ccfd2b80ce..eec1bc95391b 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3319,11 +3319,13 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
  * @desc_len: mapped desc length (out)
  */
 void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
-				  int *desc_len)
+				  int desc_index, int *desc_len)
 {
 	if (desc_id >= QUERY_DESC_IDN_MAX || desc_id == QUERY_DESC_IDN_RFU_0 ||
 	    desc_id == QUERY_DESC_IDN_RFU_1)
 		*desc_len = 0;
+	else if (desc_index == UFS_UPIU_RPMB_WLUN)
+		*desc_len = hba->desc_size[QUERY_DESC_IDN_UNIT_RPMB];
 	else
 		*desc_len = hba->desc_size[desc_id];
 }
@@ -3334,14 +3336,16 @@ static void ufshcd_update_desc_length(struct ufs_hba *hba,
 				      unsigned char desc_len)
 {
 	if (hba->desc_size[desc_id] == QUERY_DESC_MAX_SIZE &&
-	    desc_id != QUERY_DESC_IDN_STRING &&
-	    desc_index != UFS_UPIU_RPMB_WLUN)
+	    desc_id != QUERY_DESC_IDN_STRING) {
+		if (desc_index == UFS_UPIU_RPMB_WLUN)
 		/* For UFS 3.1, the normal unit descriptor is 10 bytes larger
 		 * than the RPMB unit, however, both descriptors share the same
-		 * desc_idn, to cover both unit descriptors with one length, we
-		 * choose the normal unit descriptor length by desc_index.
+		 * desc_idn, but differ by the descriptor index
 		 */
-		hba->desc_size[desc_id] = desc_len;
+			hba->desc_size[QUERY_DESC_IDN_UNIT_RPMB] = desc_len;
+		else
+			hba->desc_size[desc_id] = desc_len;
+	}
 }
 
 /**
@@ -3372,7 +3376,7 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
 		return -EINVAL;
 
 	/* Get the length of descriptor */
-	ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
+	ufshcd_map_desc_id_to_length(hba, desc_id, desc_index, &buff_len);
 	if (!buff_len) {
 		dev_err(hba->dev, "%s: Failed to get desc length\n", __func__);
 		return -EINVAL;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 971cfabc4a1e..c77bef77ec87 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1105,7 +1105,7 @@ int ufshcd_hold(struct ufs_hba *hba, bool async);
 void ufshcd_release(struct ufs_hba *hba);
 
 void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
-				  int *desc_length);
+				  int desc_index, int *desc_length);
 
 u32 ufshcd_get_local_unipro_ver(struct ufs_hba *hba);
 
-- 
2.17.1

[PATCH 3/3] scsi: ufs: Generalize ufs_is_valid_unit_desc_lun()

[Avri Altman]

ufs_is_valid_unit_desc_lun() test for specific wb offset case, and does
not verify that the requested field does not exceed the descriptor size.

So do that, and while at it, move it to ufshcd.h where it should be.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/scsi/ufs/ufs-sysfs.c |  2 +-
 drivers/scsi/ufs/ufs.h       | 19 -------------------
 drivers/scsi/ufs/ufshcd.c    |  2 +-
 drivers/scsi/ufs/ufshcd.h    | 25 +++++++++++++++++++++++++
 4 files changed, 27 insertions(+), 21 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
index 52bd807f7940..0d6dfaa70d2f 100644
--- a/drivers/scsi/ufs/ufs-sysfs.c
+++ b/drivers/scsi/ufs/ufs-sysfs.c
@@ -1136,7 +1136,7 @@ static ssize_t _pname##_show(struct device *dev,			\
 	struct scsi_device *sdev = to_scsi_device(dev);			\
 	struct ufs_hba *hba = shost_priv(sdev->host);			\
 	u8 lun = ufshcd_scsi_to_upiu_lun(sdev->lun);			\
-	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun,		\
+	if (!ufs_is_valid_unit_desc_lun(hba, lun,			\
 				_duname##_DESC_PARAM##_puname))		\
 		return -EINVAL;						\
 	return ufs_sysfs_read_desc_param(hba, QUERY_DESC_IDN_##_duname,	\
diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index d0be8d4c8091..366ece129a4d 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -571,23 +571,4 @@ enum ufs_trace_tsf_t {
 	UFS_TSF_CDB, UFS_TSF_OSF, UFS_TSF_TM_INPUT, UFS_TSF_TM_OUTPUT
 };
 
-/**
- * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit descriptor
- * @dev_info: pointer of instance of struct ufs_dev_info
- * @lun: LU number to check
- * @return: true if the lun has a matching unit descriptor, false otherwise
- */
-static inline bool ufs_is_valid_unit_desc_lun(struct ufs_dev_info *dev_info,
-		u8 lun, u8 param_offset)
-{
-	if (!dev_info || !dev_info->max_lu_supported) {
-		pr_err("Max General LU supported by UFS isn't initialized\n");
-		return false;
-	}
-	/* WB is available only for the logical unit from 0 to 7 */
-	if (param_offset == UNIT_DESC_PARAM_WB_BUF_ALLOC_UNITS)
-		return lun < UFS_UPIU_MAX_WB_LUN_ID;
-	return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported);
-}
-
 #endif /* End of Header */
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index eec1bc95391b..7f4c8f0c0459 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3555,7 +3555,7 @@ static inline int ufshcd_read_unit_desc_param(struct ufs_hba *hba,
 	 * Unit descriptors are only available for general purpose LUs (LUN id
 	 * from 0 to 7) and RPMB Well known LU.
 	 */
-	if (!ufs_is_valid_unit_desc_lun(&hba->dev_info, lun, param_offset))
+	if (!ufs_is_valid_unit_desc_lun(hba, lun, param_offset))
 		return -EOPNOTSUPP;
 
 	return ufshcd_read_desc_param(hba, QUERY_DESC_IDN_UNIT, lun,
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c77bef77ec87..395c1f5ecf9d 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1122,6 +1122,31 @@ int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
 int ufshcd_suspend_prepare(struct device *dev);
 void ufshcd_resume_complete(struct device *dev);
 
+/**
+ * ufs_is_valid_unit_desc_lun - checks if the given LUN has a unit descriptor
+ * @dev_info: pointer of instance of struct ufs_dev_info
+ * @lun: LU number to check
+ * @return: true if the lun has a matching unit descriptor, false otherwise
+ */
+static inline bool ufs_is_valid_unit_desc_lun(struct ufs_hba *hba, u8 lun,
+					      u8 param_offset)
+{
+	struct ufs_dev_info *dev_info = &hba->dev_info;
+	u8 desc_size = lun == UFS_UPIU_RPMB_WLUN ?
+			hba->desc_size[QUERY_DESC_IDN_UNIT_RPMB] :
+			hba->desc_size[QUERY_DESC_IDN_UNIT];
+
+	if (!dev_info || !dev_info->max_lu_supported) {
+		pr_err("Max General LU supported by UFS isn't initialized\n");
+		return false;
+	}
+
+	if (param_offset >= desc_size)
+		return false;
+
+	return lun == UFS_UPIU_RPMB_WLUN || (lun < dev_info->max_lu_supported);
+}
+
 /* Wrapper functions for safely calling variant operations */
 static inline const char *ufshcd_get_var_name(struct ufs_hba *hba)
 {
-- 
2.17.1

RE: [PATCH 2/3] scsi: ufs: Map the correct size to the rpmb unit descriptor

[Daejun Park]

Hi Avri,

> diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
> index 579cf6f9e7a1..d0be8d4c8091 100644
> --- a/drivers/scsi/ufs/ufs.h
> +++ b/drivers/scsi/ufs/ufs.h
> @@ -167,6 +167,7 @@ enum desc_idn {
>          QUERY_DESC_IDN_GEOMETRY                = 0x7,
>          QUERY_DESC_IDN_POWER                = 0x8,
>          QUERY_DESC_IDN_HEALTH           = 0x9,
> +        QUERY_DESC_IDN_UNIT_RPMB        = 0xA,
>          QUERY_DESC_IDN_MAX,

By adding QUERY_DESC_IDN_UNIT_RPMB, the value of QUERY_DESC_IDN_MAX is
changed to 0xB.
...

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 74ccfd2b80ce..eec1bc95391b 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3319,11 +3319,13 @@ int ufshcd_query_descriptor_retry(struct ufs_hba *hba,
>   * @desc_len: mapped desc length (out)
>   */
>  void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn desc_id,
> -                                  int *desc_len)
> +                                  int desc_index, int *desc_len)
>  {
>          if (desc_id >= QUERY_DESC_IDN_MAX || desc_id == QUERY_DESC_IDN_RFU_0 ||
>              desc_id == QUERY_DESC_IDN_RFU_1)
>                  *desc_len = 0;

So, if user sending desc_id as 0xA, it can not be detected as invalid descriptor.

> +        else if (desc_index == UFS_UPIU_RPMB_WLUN)
> +                *desc_len = hba->desc_size[QUERY_DESC_IDN_UNIT_RPMB];
>          else
>                  *desc_len = hba->desc_size[desc_id];
>  }

Thanks,
Daejun

RE: [PATCH 2/3] scsi: ufs: Map the correct size to the rpmb unit descriptor

[Avri Altman]

> Hi Avri,
> 
> > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index
> > 579cf6f9e7a1..d0be8d4c8091 100644
> > --- a/drivers/scsi/ufs/ufs.h
> > +++ b/drivers/scsi/ufs/ufs.h
> > @@ -167,6 +167,7 @@ enum desc_idn {
> >          QUERY_DESC_IDN_GEOMETRY                = 0x7,
> >          QUERY_DESC_IDN_POWER                = 0x8,
> >          QUERY_DESC_IDN_HEALTH           = 0x9,
> > +        QUERY_DESC_IDN_UNIT_RPMB        = 0xA,
> >          QUERY_DESC_IDN_MAX,
> 
> By adding QUERY_DESC_IDN_UNIT_RPMB, the value of
> QUERY_DESC_IDN_MAX is changed to 0xB.
> ...
Yes

> 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 74ccfd2b80ce..eec1bc95391b 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -3319,11 +3319,13 @@ int ufshcd_query_descriptor_retry(struct ufs_hba
> *hba,
> >   * @desc_len: mapped desc length (out)
> >   */
> >  void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn
> desc_id,
> > -                                  int *desc_len)
> > +                                  int desc_index, int *desc_len)
> >  {
> >          if (desc_id >= QUERY_DESC_IDN_MAX || desc_id ==
> QUERY_DESC_IDN_RFU_0 ||
> >              desc_id == QUERY_DESC_IDN_RFU_1)
> >                  *desc_len = 0;
> 
> So, if user sending desc_id as 0xA, it can not be detected as invalid descriptor.
Which user?
Oh, you mean if someone uses the ufs-bsg with some upiu-based query app, like ufs-utils?
Well, those apps are for developers and field engineers, expected to know what they are doing...

Alternatively, maybe its better to just remove the unit descriptor sysfs entries for wlun altogether?
They really meant nothing and shouldn't be there in the first place.
What do you think?

Thanks,
Avri 
> 
> > +        else if (desc_index == UFS_UPIU_RPMB_WLUN)
> > +                *desc_len = hba->desc_size[QUERY_DESC_IDN_UNIT_RPMB];
> >          else
> >                  *desc_len = hba->desc_size[desc_id];  }
> 
> Thanks,
> Daejun

RE:(2) [PATCH 2/3] scsi: ufs: Map the correct size to the rpmb unit descriptor

[Daejun Park]

Hi,

> > Hi Avri,
> > 
> > > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h index
> > > 579cf6f9e7a1..d0be8d4c8091 100644
> > > --- a/drivers/scsi/ufs/ufs.h
> > > +++ b/drivers/scsi/ufs/ufs.h
> > > @@ -167,6 +167,7 @@ enum desc_idn {
> > >          QUERY_DESC_IDN_GEOMETRY                = 0x7,
> > >          QUERY_DESC_IDN_POWER                = 0x8,
> > >          QUERY_DESC_IDN_HEALTH           = 0x9,
> > > +        QUERY_DESC_IDN_UNIT_RPMB        = 0xA,
> > >          QUERY_DESC_IDN_MAX,
> > 
> > By adding QUERY_DESC_IDN_UNIT_RPMB, the value of
> > QUERY_DESC_IDN_MAX is changed to 0xB.
> > ...
> Yes
>  
> > 
> > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > > index 74ccfd2b80ce..eec1bc95391b 100644
> > > --- a/drivers/scsi/ufs/ufshcd.c
> > > +++ b/drivers/scsi/ufs/ufshcd.c
> > > @@ -3319,11 +3319,13 @@ int ufshcd_query_descriptor_retry(struct ufs_hba
> > *hba,
> > >   * @desc_len: mapped desc length (out)
> > >   */
> > >  void ufshcd_map_desc_id_to_length(struct ufs_hba *hba, enum desc_idn
> > desc_id,
> > > -                                  int *desc_len)
> > > +                                  int desc_index, int *desc_len)
> > >  {
> > >          if (desc_id >= QUERY_DESC_IDN_MAX || desc_id ==
> > QUERY_DESC_IDN_RFU_0 ||
> > >              desc_id == QUERY_DESC_IDN_RFU_1)
> > >                  *desc_len = 0;
> > 
> > So, if user sending desc_id as 0xA, it can not be detected as invalid descriptor.
> Which user?
> Oh, you mean if someone uses the ufs-bsg with some upiu-based query app, like ufs-utils?
> Well, those apps are for developers and field engineers, expected to know what they are doing...

Yes, but checking QUERY_DESC_IDN_MAX can be useless because of adding entry in enum desc_idn.
 
> Alternatively, maybe its better to just remove the unit descriptor sysfs entries for wlun altogether?
> They really meant nothing and shouldn't be there in the first place.
> What do you think?

Although if they were removed, ufs-bsg can access unit descriptors of wlun.
But it can be OK because developers are expected to access unit descriptors of wlun correctly.
So, I think it can be a solution.

> Thanks,
> Avri 
> > 
> > > +        else if (desc_index == UFS_UPIU_RPMB_WLUN)
> > > +                *desc_len = hba->desc_size[QUERY_DESC_IDN_UNIT_RPMB];
> > >          else
> > >                  *desc_len = hba->desc_size[desc_id];  }
> > 
> > Thanks,
> > Daejun
>  
>  
>  

Re: [PATCH 1/3] scsi: ufs: Remove redundant define

[Bean Huo]

On Tue, 2021-07-27 at 15:35 +0300, Avri Altman wrote:
> UFS_UPIU_RPMB_WLUN already describe the rpmb wlun index.
> 
> 
> 
> Signed-off-by: Avri Altman <avri.altman@wdc.com>

Reviewed-by: Bean Huo <beanhuo@micron.com>

Re: [PATCH 0/3] Log correct rpmb unit descriptor size

[Martin K. Petersen]

Avri,

> For the rpmb unit descriptor, if the field offset is larger than 0x23,
> it may trigger a stack corruption because a) we do not log properly the
> rpmb unit descriptor size,  and b) ufs_is_valid_unit_desc_lun() test for
> specific wb offset case, and does not verify that the requested field
> does not exceed the descriptor size.

Please rebase on top of 5.15/scsi-staging and fix the resulting
errors. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering