linux-kernel - RE: Re: [PATCH v2 1/1] scsi: ufs: Fix unexpected values get from ufshcd_read_desc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 30 Oct 2020 16:19:52 +0900
From:   Daejun Park <daejun7.park@...sung.com>
To:     Can Guo <cang@...eaurora.org>,
        Daejun Park <daejun7.park@...sung.com>,
        "avri.altman@....com" <avri.altman@....com>
CC:     ALIM AKHTAR <alim.akhtar@...sung.com>,
        "asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
        "beanhuo@...ron.com" <beanhuo@...ron.com>,
        "bvanassche@....org" <bvanassche@....org>,
        "hongwus@...eaurora.org" <hongwus@...eaurora.org>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "kernel-team@...roid.com" <kernel-team@...roid.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "nguyenb@...eaurora.org" <nguyenb@...eaurora.org>,
        "rnayak@...eaurora.org" <rnayak@...eaurora.org>,
        "salyzyn@...gle.com" <salyzyn@...gle.com>,
        "saravanak@...gle.com" <saravanak@...gle.com>,
        "stanley.chu@...iatek.com" <stanley.chu@...iatek.com>
Subject: RE: Re: [PATCH v2 1/1] scsi: ufs: Fix unexpected values get from
 ufshcd_read_desc_param()

>> 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@...eaurora.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
>
>Yes, I considered that during bootup time, but the current driver won't 
>even
>access WB related stuffs it is not supported (there are checks against 
>UFS version
>and feature supports in ufshcd_wb_probe()). So this change is only 
>proecting illegal
>access from sysfs entries after bootup is done. Do you see real error 
>during bootup
>time? If yes, please let me know.
>
>Thanks,
>
>Can Guo.

Can Guo,
I haven't seen any real errors. If it's meant to prevent erroneous access
from sysfs, it seems good enough.

Acked-by: Daejun Park <daejun7.park@...sung.com>

Avri,
ufshcd_is_wb_attrs or ufshcd_is_wb_flag is used to match appropriate lun
in case of shared lu WB. I think Can Guo's code is suitable to prevent
wrong access to descriptors.

Thanks,
Daejun