Re: [PATCH] scsi: ufs: pltfrm: fix use-after free in init error and remove paths

[André Draszik]

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'