Re: [PATCH v8 15/15] PCI/TSM: Wait for users during unregister

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] PCI/TSM: use-after-free in tsm_unregister() and sev_tsm_uninit()
> - [Low] PCI/TSM: mixing guard() and goto in pci_tsm_context_get() and pci_tsm_init()
> --
>
> commit b2546a16b6b6a681e70bba7a7e4c0fbd2665c584
> Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
>
> PCI/TSM: Wait for users during unregister
>
> This commit adds registration_lock and a user reference count to struct
> tsm_dev so that tsm_unregister() and pci_tsm_destroy() wait for active TSM
> users, including IOMMUFD virtual IOMMUs without vDEVICE objects, before
> tearing down PCI connections.
>
>> diff --git a/drivers/crypto/ccp/sev-dev-tsm.c b/drivers/crypto/ccp/sev-dev-tsm.c
>> index 46f2539d2d5ab..5c61ce74ee0f7 100644
>> --- a/drivers/crypto/ccp/sev-dev-tsm.c
>> +++ b/drivers/crypto/ccp/sev-dev-tsm.c
>> @@ -392,8 +392,8 @@ void sev_tsm_init_locked(struct sev_device *sev, void *tio_status_page)
>>  
>>  void sev_tsm_uninit(struct sev_device *sev)
>>  {
>> -	if (sev->tsmdev)
>> -		tsm_unregister(sev->tsmdev);
>> +	if (sev->tsmdev && WARN_ON(tsm_unregister(sev->tsmdev)))
>> +		return;
>>  
>>  	sev->tsmdev = NULL;
>>  }
>
> [Severity: High]
> Can a concurrent call to sev_tsm_uninit() while tsm_unregister() is waiting
> for TSM users trigger a use-after-free or premature teardown?
>
> In drivers/crypto/ccp/sev-dev.c, sev_firmware_shutdown() invokes
> sev_tsm_uninit() outside sev_cmd_mutex:
>
> drivers/crypto/ccp/sev-dev.c:sev_firmware_shutdown() {
> 	if (sev->tio_status)
> 		sev_tsm_uninit(sev);
>
> 	sev_snp_unregister_verify_mitigation(sev);
>
> 	mutex_lock(&sev_cmd_mutex);
>
> 	__sev_firmware_shutdown(sev, false);
>
> 	kfree(sev->tio_status);
> 	sev->tio_status = NULL;
>
> 	mutex_unlock(&sev_cmd_mutex);
> }
>
> Because tsm_unregister() now sleeps in tsm_wait_for_users() and
> sev->tsmdev is only cleared after tsm_unregister() returns, a concurrent
> caller of sev_firmware_shutdown() (such as sev_platform_shutdown() racing
> with sev_dev_destroy() or sev_pci_exit()) can see non-NULL sev->tio_status
> and sev->tsmdev and call tsm_unregister(sev->tsmdev) a second time.
>
> Additionally, if tsm_unregister() returns -ENODEV here, sev_tsm_uninit()
> returns early without clearing sev->tsmdev, and sev_firmware_shutdown()
> proceeds to run __sev_firmware_shutdown() and free sev->tio_status while
> the first caller is still blocked in tsm_wait_for_users() or disconnecting
> PCI devices in pci_tsm_unregister().
>

 [ ... 132 lines skipped ... ] 

>
> [Severity: High]
> Can the tsm_dev->unregistering check in tsm_unregister() cause a
> use-after-free on tsm_dev when two callers race to unregister the same
> device?
>
> tsm_register() initializes tsm_dev->dev with a single registration device
> reference. When a first caller enters tsm_unregister(), it holds
> down_write(&tsm_dev->registration_lock) across device_del(),
> tsm_wait_for_users(), and pci_tsm_unregister().
>
> If a second caller enters tsm_unregister() while the first caller is
> waiting for users to drain, the second caller blocks in
> down_write(&tsm_dev->registration_lock) without holding a device reference
> on tsm_dev->dev. Once all user references have been dropped and the first
> caller finishes:
>
> 	up_write(&tsm_dev->registration_lock);
> 	put_device(&tsm_dev->dev);
>
> put_device(&tsm_dev->dev) drops the final reference and frees tsm_dev via
> tsm_release(), while the second caller wakes from
> down_write(&tsm_dev->registration_lock), reads tsm_dev->unregistering, and
> calls up_write(&tsm_dev->registration_lock) on the freed tsm_dev.
>

I consider both issues pre-existing. The SEV code should ensure that
concurrent calls to tsm_unregister() hold additional references so that
tsm_dev remains valid. Can this be fixed separately from this series?

-aneesh