linux-kernel - Re: [PATCH v2] scsi: ufshcd: fix possible unclocked register access

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:   Thu, 06 Oct 2016 18:20:03 -0700
From:   Subhash Jadavani <subhashj@...eaurora.org>
To:     Kiwoong Kim <kwmad.kim@...sung.com>
Cc:     vinholikatti@...il.com, jejb@...ux.vnet.ibm.com,
        martin.petersen@...cle.com,
        "'open list:UNIVERSAL FLASH STORAGE HOST CONTROLLER DRIVER'" 
        <linux-scsi@...r.kernel.org>,
        'open list' <linux-kernel@...r.kernel.org>,
        linux-scsi-owner@...r.kernel.org
Subject: Re: [PATCH v2] scsi: ufshcd: fix possible unclocked register access

On 2016-10-06 18:16, Kiwoong Kim wrote:
> Hi, Subhash.
> 
>> Thanks Kim for the response.
>> 
>> On 2016-10-06 03:28, Kiwoong Kim wrote:
>> > Hi, Subhash.
>> >
>> > Some UFS host controllers may need to call the vendor specific
>> > callback before and after controlling by clock control framework,
>> > regardless of whether available clocks are turned on or off.
>> 
>> Are you suggesting to call ufshcd_vops_setup_clocks() 2 times, one 
>> before
>> the on/off by ufshcd core driver and one after the on/off? If yes, 
>> then we
>> also have add 3rd argument clarifying if this is PRE_CHANGE or
> POST_CHANGE.
>> 
>> >
>> > Is there any special reason to limit to invoke the callback
>> > only when the clocks are turned on or not?
>> >
>> > Besides, the callback is acknowledged from core driver
>> > because 2nd argument is whether the clocks are turned on or not.
>> >
>> > If you have any other idea, please let me know.
>> 
>> This is my suggestion:
>> 1. Add 3rd argument to setup_clocks ops to let the vendor callback
>> function know if this is called PRE_CHANGE or POST_CHANGE.
>> 2. If #1 is in place, call setup_clocks 2 times, one with PRE_CHANGE
>> argument before making any clock changes in core driver, 2nd with
>> POST_CHANGE argument after making the clock changes to core driver.
> 
> Yes, that's exactly the same what I mean.

Thanks, Will send out new version adding that.

> 
> Thanks
> 
> 
>> 
>> Let me know if this would work or not.
>> 
>> Thanks,
>> Subhash
>> 
>> >
>> > Thanks
>> > Regards
>> >
>> >> Vendor specific setup_clocks callback may require the clocks managed
>> >> by ufshcd driver to be ON. So if the vendor specific setup_clocks
>> >> callback
>> >> is called while the required clocks are turned off, it could result
>> >> into
>> >> unclocked register access.
>> >>
>> >> To prevent possible unclock register access, this change makes sure
>> >> that
>> >> required clocks remain enabled before calling into vendor specific
>> >> setup_clocks callback.
>> >>
>> >> Signed-off-by: Subhash Jadavani <subhashj@...eaurora.org>
>> >> ---
>> >> Changes from v2:
>> >> * Don't call ufshcd_vops_setup_clocks() again for clock off
>> >> ---
>> >>  drivers/scsi/ufs/ufshcd.c | 22 +++++++++++++++++++++-
>> >>  1 file changed, 21 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> >> index 05c7456..c1a77d3 100644
>> >> --- a/drivers/scsi/ufs/ufshcd.c
>> >> +++ b/drivers/scsi/ufs/ufshcd.c
>> >> @@ -5389,6 +5389,17 @@ static int __ufshcd_setup_clocks(struct ufs_hba
>> >> *hba, bool on,
>> >>  	if (!head || list_empty(head))
>> >>  		goto out;
>> >>
>> >> +	/*
>> >> +	 * vendor specific setup_clocks ops may depend on clocks managed by
>> >> +	 * this standard driver hence call the vendor specific setup_clocks
>> >> +	 * before disabling the clocks managed here.
>> >> +	 */
>> >> +	if (!on) {
>> >> +		ret = ufshcd_vops_setup_clocks(hba, on);
>> >> +		if (ret)
>> >> +			return ret;
>> >> +	}
>> >> +
>> >>  	list_for_each_entry(clki, head, list) {
>> >>  		if (!IS_ERR_OR_NULL(clki->clk)) {
>> >>  			if (skip_ref_clk && !strcmp(clki->name, "ref_clk"))
>> >> @@ -5410,7 +5421,16 @@ static int __ufshcd_setup_clocks(struct ufs_hba
>> >> *hba, bool on,
>> >>  		}
>> >>  	}
>> >>
>> >> -	ret = ufshcd_vops_setup_clocks(hba, on);
>> >> +	/*
>> >> +	 * vendor specific setup_clocks ops may depend on clocks managed by
>> >> +	 * this standard driver hence call the vendor specific setup_clocks
>> >> +	 * after enabling the clocks managed here.
>> >> +	 */
>> >> +	if (on) {
>> >> +		ret = ufshcd_vops_setup_clocks(hba, on);
>> >> +		if (ret)
>> >> +			return ret;
>> >> +	}
>> >>  out:
>> >>  	if (ret) {
>> >>  		list_for_each_entry(clki, head, list) {
>> >> --
>> >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>> >> Forum,
>> >> a Linux Foundation Collaborative Project
>> >>
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
>> >> in
>> >> the body of a message to majordomo@...r.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-scsi"
>> > in
>> > the body of a message to majordomo@...r.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
>> --
>> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" 
>> in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" 
> in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project