On Mon, Jan 13, 2025 at 07:13:45PM +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 ufshd 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: > > exynos-ufshc 14700000.ufs: ufshcd_pltfrm_init() failed -11 > exynos-ufshc 14700000.ufs: probe with driver exynos-ufshc failed with error -11 > Unable to handle kernel paging request at virtual address 01adafad6dadad88 > Mem abort info: > ESR = 0x0000000096000004 > EC = 0x25: DABT (current EL), IL = 32 bits > SET = 0, FnV = 0 > EA = 0, S1PTW = 0 > FSC = 0x04: level 0 translation fault > Data abort info: > ISV = 0, ISS = 0x00000004, ISS2 = 0x00000000 > CM = 0, WnR = 0, TnD = 0, TagAccess = 0 > GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 > [01adafad6dadad88] address between user and kernel address ranges > Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP > Modules linked in: > CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G W 6.13.0-rc5-next-20250106+ #70 > Tainted: [W]=WARN > Hardware name: Oriole (DT) > pstate: 20400005 (nzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--) > pc : kfree+0x60/0x2d8 > lr : kvfree+0x44/0x60 > sp : ffff80008009ba80 > x29: ffff80008009ba90 x28: 0000000000000000 x27: ffffbcc6591e0130 > x26: ffffbcc659309960 x25: ffffbcc658f89c50 x24: ffffbcc659539d80 > x23: ffff22e000940040 x22: ffff22e001539010 x21: ffffbcc65714b22c > x20: 6b6b6b6b6b6b6b6b x19: 01adafad6dadad80 x18: 0000000000000000 > x17: ffffbcc6579fbac8 x16: ffffbcc657a04300 x15: ffffbcc657a027f4 > x14: ffffbcc656f969cc x13: ffffbcc6579fdc80 x12: ffffbcc6579fb194 > x11: ffffbcc6579fbc34 x10: 0000000000000000 x9 : ffffbcc65714b22c > x8 : ffff80008009b880 x7 : 0000000000000000 x6 : ffff80008009b940 > x5 : ffff80008009b8c0 x4 : ffff22e000940518 x3 : ffff22e006f54f40 > x2 : ffffbcc657a02268 x1 : ffff80007fffffff x0 : ffffc1ffc0000000 > 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, register the crypto cleanup handler against > the scsi-host device instead, so that it runs before release of struct > ufs_hba. > > Signed-off-by: André Draszik <andre.draszik@xxxxxxxxxx> > --- > In my case, as per above trace I initially encountered an error in > ufshcd_verify_dev_init(), which made me notice this problem. For > reproducing, it'd be possible to change that function to just return an > error. > > Other approaches for solving this issue I see are the following, but I > believe this one here is the cleanest: > > * turn 'struct ufs_hba::crypto_profile' into a dynamically allocated > pointer, in which case it doesn't matter if cleanup runs after > scsi_host_put() > * add an explicit devm_blk_crypto_profile_deinit() to be called by API > users when necessary, e.g. before ufshcd_dealloc_host() in this case Thanks for catching this. There's also the option of using blk_crypto_profile_init() instead of devm_blk_crypto_profile_init(), and calling blk_crypto_profile_destroy() explicitly. Would that be any easier here? Please also include a Fixes tag in the commit message. - Eric
