On Fri, Jan 24, 2025 at 03:09:00PM +0000, André Draszik wrote:
> devm_blk_crypto_profile_init() registers a cleanup handler to run when
> the associated (platform-) device is being released. For UFS, the
> crypto private data and pointers are stored as part of the ufs_hba's
> data structure 'struct ufs_hba::crypto_profile'. This structure is
> allocated as part of the underlying ufshcd and therefore Scsi_host
> allocation.
>
> During driver release or during error handling in ufshcd_pltfrm_init(),
> this structure is released as part of ufshcd_dealloc_host() before the
> (platform-) device associated with the crypto call above is released.
> Once this device is released, the crypto cleanup code will run, using
> the just-released 'struct ufs_hba::crypto_profile'. This causes a
> use-after-free situation:
>
> Call trace:
> kfree+0x60/0x2d8 (P)
> kvfree+0x44/0x60
> blk_crypto_profile_destroy_callback+0x28/0x70
> devm_action_release+0x1c/0x30
> release_nodes+0x6c/0x108
> devres_release_all+0x98/0x100
> device_unbind_cleanup+0x20/0x70
> really_probe+0x218/0x2d0
>
> In other words, the initialisation code flow is:
>
> platform-device probe
> ufshcd_pltfrm_init()
> ufshcd_alloc_host()
> scsi_host_alloc()
> allocation of struct ufs_hba
> creation of scsi-host devices
> devm_blk_crypto_profile_init()
> devm registration of cleanup handler using platform-device
>
> and during error handling of ufshcd_pltfrm_init() or during driver
> removal:
>
> ufshcd_dealloc_host()
> scsi_host_put()
> put_device(scsi-host)
> release of struct ufs_hba
> put_device(platform-device)
> crypto cleanup handler
>
> To fix this use-after free, change ufshcd_alloc_host() to register a
> devres action to automatically cleanup the underlying SCSI device on
> ufshcd destruction, without requiring explicit calls to
> ufshcd_dealloc_host(). This way:
>
> * the crypto profile and all other ufs_hba-owned resources are
> destroyed before SCSI (as they've been registered after)
> * a memleak is plugged in tc-dwc-g210-pci.c remove() as a
> side-effect
> * EXPORT_SYMBOL_GPL(ufshcd_dealloc_host) can be removed fully as
> it's not needed anymore
> * no future drivers using ufshcd_alloc_host() could ever forget
> adding the cleanup
>
> Fixes: cb77cb5abe1f ("blk-crypto: rename blk_keyslot_manager to blk_crypto_profile")
> Fixes: d76d9d7d1009 ("scsi: ufs: use devm_blk_ksm_init()")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: André Draszik <andre.draszik@xxxxxxxxxx>
Acked-by: Eric Biggers <ebiggers@xxxxxxxxxx>
- Eric
