Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Mime-Version: 1.0
Subject: RE: [PATCH v2 1/1] scsi: ufs: Fix unexpected values get from
 ufshcd_read_desc_param()
Reply-To: daejun7.park@samsung.com
Sender: Daejun Park <daejun7.park@samsung.com>
From:   Daejun Park <daejun7.park@samsung.com>
To:     "cang@codeaurora.org" <cang@codeaurora.org>
CC:     ALIM AKHTAR <alim.akhtar@samsung.com>,
        "asutoshd@codeaurora.org" <asutoshd@codeaurora.org>,
        "avri.altman@wdc.com" <avri.altman@wdc.com>,
        "beanhuo@micron.com" <beanhuo@micron.com>,
        "bvanassche@acm.org" <bvanassche@acm.org>,
        "hongwus@codeaurora.org" <hongwus@codeaurora.org>,
        "jejb@linux.ibm.com" <jejb@linux.ibm.com>,
        "kernel-team@android.com" <kernel-team@android.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        "nguyenb@codeaurora.org" <nguyenb@codeaurora.org>,
        "rnayak@codeaurora.org" <rnayak@codeaurora.org>,
        "salyzyn@google.com" <salyzyn@google.com>,
        "saravanak@google.com" <saravanak@google.com>,
        "stanley.chu@mediatek.com" <stanley.chu@mediatek.com>
Message-ID: <963815509.21603435202191.JavaMail.epsvc@epcpadp1>
Date:   Fri, 23 Oct 2020 15:35:28 +0900
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
References: <CGME20201023063528epcms2p11b57d929a926d582539ce4e1a57caf80@epcms2p1>
Precedence: bulk

Hi, Can Guo

>Since WB feature has been added, WB related sysfs entries can be accessed
>even when an UFS device does not support WB feature. In that case, the
>descriptors which are not supported by the UFS device may be wrongly
>reported when they are accessed from their corrsponding sysfs entries.
>Fix it by adding a sanity check of parameter offset against the actual
>decriptor length.
>
>Signed-off-by: Can Guo <cang@codeaurora.org>
>---
> drivers/scsi/ufs/ufshcd.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>index a2ebcc8..aeec10d 100644
>--- a/drivers/scsi/ufs/ufshcd.c
>+++ b/drivers/scsi/ufs/ufshcd.c
>@@ -3184,13 +3184,19 @@ int ufshcd_read_desc_param(struct ufs_hba *hba,
> 	/* Get the length of descriptor */
> 	ufshcd_map_desc_id_to_length(hba, desc_id, &buff_len);
> 	if (!buff_len) {
>-		dev_err(hba->dev, "%s: Failed to get desc length", __func__);
>+		dev_err(hba->dev, "%s: Failed to get desc length\n", __func__);
>+		return -EINVAL;
>+	}
>+
>+	if (param_offset >= buff_len) {
>+		dev_err(hba->dev, "%s: Invalid offset 0x%x in descriptor IDN 0x%x, length 0x%x\n",
>+			__func__, param_offset, desc_id, buff_len);

In my understanding, this code seems to check incorrect access to not
supportted features (e.g. WB) via buff_len value from
ufshcd_map_desc_id_to_length().
However, since buff_len is initialized as QUERY_DESC_MAX_SIZE and is
updated later by ufshcd_update_desc_length(), So it is impossible to find
incorrect access by checking buff_len at first time.

Thanks,
Daejun