| Message ID | 20250113-ufshcd-fix-v1-1-ca63d1d4bd55@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: pltfrm: fix use-after free in init error and remove paths | expand |
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@linaro.org> > --- > 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
On Mon, 2025-01-13 at 19:22 +0000, Eric Biggers wrote: > 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@linaro.org> > > --- > > 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? Ah, yes, that was actually my initial fix for testing, but I dismissed that due to needing more changes and potentially not knowing in all situation if it needs to be called or not. TBH, my preferred fix would actually be the alternative 1 outlined above (dynamic allocation). This way future drivers can not make this same mistake. Any thoughts? > Please also include a Fixes tag in the commit message. Will do. Cheers, Andre'
On Mon, Jan 13, 2025 at 07:28:00PM +0000, André Draszik wrote: > On Mon, 2025-01-13 at 19:22 +0000, Eric Biggers wrote: > > 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@linaro.org> > > > --- > > > 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? > > Ah, yes, that was actually my initial fix for testing, but I dismissed > that due to needing more changes and potentially not knowing in all > situation if it needs to be called or not. > > TBH, my preferred fix would actually be the alternative 1 outlined > above (dynamic allocation). This way future drivers can not make this > same mistake. > > Any thoughts? > I assume you mean replacing devm_blk_crypto_profile_init() with a new function devm_blk_crypto_profile_new() that dynamically allocates a struct blk_crypto_profile, and making struct ufs_hba store a pointer to struct blk_crypto_profile instead of embed it. And likewise for struct mmc_host. I think that would avoid this bug, but it seems suboptimal to introduce the extra level of indirection. The blk_crypto_profile is not really an independent object from struct ufs_hba; all its methods need to cast the blk_crypto_profile to the struct ufs_hba that contains it. We could replace the container_of() with a back-pointer, so technically it would work. But the fact that both would be pointing to each other suggests that they really should be in the same struct. If it's possible, it would be nice to just make the destruction of the crypto profile happen at the right time, when the other resources owned by the ufs_hba are destroyed. - Eric
On Mon, 2025-01-13 at 20:31 +0000, Eric Biggers wrote: > On Mon, Jan 13, 2025 at 07:28:00PM +0000, André Draszik wrote: > > On Mon, 2025-01-13 at 19:22 +0000, Eric Biggers wrote: > > > On Mon, Jan 13, 2025 at 07:13:45PM +0000, André Draszik wrote: > > > > [...] > > > > ther 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? > > > > Ah, yes, that was actually my initial fix for testing, but I dismissed > > that due to needing more changes and potentially not knowing in all > > situation if it needs to be called or not. > > > > TBH, my preferred fix would actually be the alternative 1 outlined > > above (dynamic allocation). This way future drivers can not make this > > same mistake. > > > > Any thoughts? > > > > I assume you mean replacing devm_blk_crypto_profile_init() with a new function > devm_blk_crypto_profile_new() that dynamically allocates a struct > blk_crypto_profile, and making struct ufs_hba store a pointer to struct > blk_crypto_profile instead of embed it. And likewise for struct mmc_host. Yes, but I was only thinking of ufs_hba since I believe mmc_host uses an approach similar to my patch, where the lifetime of the crypto devm cleanup is bound to the underlying mmc device. For consistency reasons, I guess both would have to be changed indeed. > I think that would avoid this bug, but it seems suboptimal to introduce the > extra level of indirection. The blk_crypto_profile is not really an independent > object from struct ufs_hba; all its methods need to cast the blk_crypto_profile > to the struct ufs_hba that contains it. We could replace the container_of() > with a back-pointer, so technically it would work. But the fact that both would > be pointing to each other suggests that they really should be in the same > struct. Noted. Thanks for your thoughts. > If it's possible, it would be nice to just make the destruction of the crypto > profile happen at the right time, when the other resources owned by the ufs_hba > are destroyed. Just to clarify, are you saying you'd prefer to rather see something like you mentioned above (calling blk_crypto_profile_destroy() explicitly)? I had a quick look, and it seems harder than this patch: one option could be to make calling the destroy dependent on UFSHCD_CAP_CRYPTO, but ufs-mediatek.c and ufs-sprd.c appear to clear that flag on errors at runtime, so we might miss a necessary cleanup call. I think the best alternative to keep devm-based crypto profile destruction, and to make it happen while other ufs_hba-owned resources are destroyed, and before SCSI destruction, is to 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(). I am not sure if this has wider implications (in particular if there is any underlying assumption or requirement for the Scsi_host device to clean up before the ufshcd device), but this way: * the crypto profile would be destroyed before SCSI (as it's registered after) * a memleak would be plugged in tc-dwc-g210-pci.c * EXPORT_SYMBOL_GPL(ufshcd_dealloc_host) could be removed fully * no future drivers using ufshcd_alloc_host() could ever forget adding the cleanup Something like the following in ufshcd.c: static void ufshcd_scsi_host_put_callback(void *host) { scsi_host_put(host); } and in ufshcd_alloc_host() just after scsi_host_alloc() in same file: err = devm_add_action_or_reset(dev, ufshcd_scsi_host_put_callback, host); if (err) return dev_err_probe(dev, err, "failed to add ufshcd dealloc action\n"); I'll change v2 to do just that. Cheers, Andre'
diff --git a/drivers/ufs/core/ufshcd-crypto.c b/drivers/ufs/core/ufshcd-crypto.c index 694ff7578fc1..cb9db79ca185 100644 --- a/drivers/ufs/core/ufshcd-crypto.c +++ b/drivers/ufs/core/ufshcd-crypto.c @@ -177,7 +177,7 @@ int ufshcd_hba_init_crypto_capabilities(struct ufs_hba *hba) /* The actual number of configurations supported is (CFGC+1) */ err = devm_blk_crypto_profile_init( - hba->dev, &hba->crypto_profile, + &hba->host->shost_gendev, &hba->crypto_profile, hba->crypto_capabilities.config_count + 1); if (err) goto out; diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c index 13dd5dfc03eb..6874c769b697 100644 --- a/drivers/ufs/host/ufs-exynos.c +++ b/drivers/ufs/host/ufs-exynos.c @@ -1312,7 +1312,8 @@ static void exynos_ufs_fmp_init(struct ufs_hba *hba, struct exynos_ufs *ufs) } /* Advertise crypto capabilities to the block layer. */ - err = devm_blk_crypto_profile_init(hba->dev, profile, 0); + err = devm_blk_crypto_profile_init(&hba->host->shost_gendev, + profile, 0); if (err) { /* Only ENOMEM should be possible here. */ dev_err(hba->dev, "Failed to initialize crypto profile: %d\n", diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 23b9f6efa047..d0a1e0f20a2f 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -141,7 +141,8 @@ static int ufs_qcom_ice_init(struct ufs_qcom_host *host) caps.reg_val = cpu_to_le32(ufshcd_readl(hba, REG_UFS_CCAP)); /* The number of keyslots supported is (CFGC+1) */ - err = devm_blk_crypto_profile_init(dev, profile, caps.config_count + 1); + err = devm_blk_crypto_profile_init(&hba->host->shost_gendev, profile, + caps.config_count + 1); if (err) return err;
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@linaro.org> --- 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 --- drivers/ufs/core/ufshcd-crypto.c | 2 +- drivers/ufs/host/ufs-exynos.c | 3 ++- drivers/ufs/host/ufs-qcom.c | 3 ++- 3 files changed, 5 insertions(+), 3 deletions(-) --- base-commit: 4e16367cfe0ce395f29d0482b78970cce8e1db73 change-id: 20250113-ufshcd-fix-52409f2d32ff Best regards,