linux-kernel - Re: [PATCH] ufs: crypto: add host_sem lock in ufshcd_program

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250321074524.126338-1-zhanghui31@xiaomi.com>
Date: Fri, 21 Mar 2025 15:45:23 +0800
From: ZhangHui <zhanghui31@...omi.com>
To: <ebiggers@...nel.org>
CC: <James.Bottomley@...senpartnership.com>, <alim.akhtar@...sung.com>,
	<avri.altman@....com>, <bvanassche@....org>, <linux-kernel@...r.kernel.org>,
	<linux-scsi@...r.kernel.org>, <martin.petersen@...cle.com>,
	<peter.griffin@...aro.org>, <zhanghui31@...omi.com>
Subject: Re: [PATCH] ufs: crypto: add host_sem lock in ufshcd_program_key

On Thu, Mar 20, 2025 at 09:44:55PM -0700, Eric Biggers wrote:
> It seems broken that the filesystem doesn't get unmounted until after the UFS is
> shut down.  It would be helpful to get a clearer picture of exactly why things
> are happening in that order.
> 
> But disregarding that, it's indeed logical for blk_crypto_evict_key() to return
> an error if it cannot fulfill the request.
> 
> But I'm wondering if this needs to be solved in the UFS driver itself or whether
> the blk-crypto framework should handle this (so that it also gets fixed for
> other drivers that may have the same problem).  In block/blk-crypto-profile.c,
> pm_runtime_get_sync() is already called before ->keyslot_evict.  So
> ->keyslot_evict is supposed to be called only when the device is resumed.
> 
> The blk-crypto code (in blk_crypto_hw_enter()) doesn't check the return value of
> pm_runtime_get_sync(), though.  That seems like a bug.  Is it possible this
> issue would be fixed if it checked the return value?
> 

hi Eric,

I have checked the device_shutdown process and it seems only wait for the resume
that has not been processed to be completed, and then continue. It does not seem
to cause pm_runtime_get_sync to return an error.

> Or does the UFS driver still need to check ufshcd_is_user_access_allowed() too?
> If that's the case, I'm also wondering whether it's okay to nest host_sem inside
> pm_runtime_get_sync().  Elsewhere in the UFS driver they are called in the
> opposite order.

I found that ufshcd_is_user_access_allowed is used in many places in the ufs driver
code. What is the historical reason for this?

thanks
zhanghui