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: <20250321044455.GB98513@sol.localdomain>
Date: Thu, 20 Mar 2025 21:44:55 -0700
From: Eric Biggers <ebiggers@...nel.org>
To: ZhangHui <zhanghui31@...omi.com>
Cc: bvanassche@....org, alim.akhtar@...sung.com, avri.altman@....com,
	James.Bottomley@...senpartnership.com, martin.petersen@...cle.com,
	peter.griffin@...aro.org, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ufs: crypto: add host_sem lock in ufshcd_program_key

Hi ZhangHui,

On Mon, Mar 17, 2025 at 07:01:01PM +0800, ZhangHui wrote:
> From: zhanghui <zhanghui31@...omi.com>
> 
> On Android devices, we found that there is a probability that
> the ufs has been shut down but the thread is still deleting the
> key, which will cause the bus error.
> 
> We checked the Android reboot process and found that it is indeed
> possible that some threads have not been killed before the device
> shutdown, because the Android reboot process will not wait until
> all threads are killed before continuing to execute.
> 
> The call stack is as follows:
> 
> __blk_crypto_evict_key+0x148/0x1c4
> blk_crypto_evict_key+0x38/0x9c
> dm_keyslot_evict_callback+0x18/0x2c
> default_key_iterate_devices+0x40/0x50
> dm_keyslot_evict+0xac/0xfc
> __blk_crypto_evict_key+0xf4/0x1c4
> blk_crypto_evict_key+0x38/0x9c
> fscrypt_destroy_inline_crypt_key+0xb8/0x10c
> fscrypt_destroy_prepared_key+0x30/0x48
> fscrypt_put_master_key_activeref+0xc4/0x308
> fscrypt_destroy_keyring+0xb0/0xfc
> generic_shutdown_super+0x60/0x12c
> kill_block_super+0x1c/0x48
> kill_f2fs_super+0xc4/0x1a8
> deactivate_locked_super+0x98/0x144
> deactivate_super+0x78/0x8c
> cleanup_mnt+0x110/0x148
> __cleanup_mnt+0x14/0x20
> task_work_run+0xc4/0xec
> get_signal+0x6c/0x8d8
> do_notify_resume+0x128/0x828
> el0_svc+0x6c/0x70
> el0t_64_sync_handler+0x68/0xbc
> el0t_64_sync+0x1a8/0x1ac
>
> Let's fixed this issue by adding a lock in program_key flow.
> 
> Signed-off-by: zhanghui <zhanghui31@...omi.com>
> ---
>  drivers/ufs/core/ufshcd-crypto.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd-crypto.c b/drivers/ufs/core/ufshcd-crypto.c
> index 694ff7578fc1..f3260a072c0c 100644
> --- a/drivers/ufs/core/ufshcd-crypto.c
> +++ b/drivers/ufs/core/ufshcd-crypto.c
> @@ -5,6 +5,7 @@
>  
>  #include <ufs/ufshcd.h>
>  #include "ufshcd-crypto.h"
> +#include "ufshcd-priv.h"
>  
>  /* Blk-crypto modes supported by UFS crypto */
>  static const struct ufs_crypto_alg_entry {
> @@ -17,12 +18,18 @@ static const struct ufs_crypto_alg_entry {
>  	},
>  };
>  
> -static void ufshcd_program_key(struct ufs_hba *hba,
> +static int ufshcd_program_key(struct ufs_hba *hba,
>  			       const union ufs_crypto_cfg_entry *cfg, int slot)
>  {
>  	int i;
>  	u32 slot_offset = hba->crypto_cfg_register + slot * sizeof(*cfg);
>  
> +	down(&hba->host_sem);
> +	if (!ufshcd_is_user_access_allowed(hba)) {
> +		up(&hba->host_sem);
> +		return -EBUSY;
> +	}
> +

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?

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.

- Eric