Re: [PATCH v3] crypto/ccp: Introduce SNP_VERIFY_MITIGATION command

[Pratik R. Sampat]

On 5/21/26 4:04 PM, Tom Lendacky wrote:
> On 5/21/26 10:05, Tycho Andersen wrote:
>> On Thu, May 21, 2026 at 08:12:52AM -0500, Tom Lendacky wrote:
>>>> Now, with unregister no longer protected by sev_cmd_mutex, a concurrent init
>>>> can race with shutdown on the sysfs lifetime like so:
>>>
>>> Can it? Can init and shutdown race? Isn't that part of module load /
>>> unload, I'm not sure how they can race...
>>
>> That's only true after
>> https://lore.kernel.org/all/20260504165147.1615643-5-tycho@xxxxxxxxxx/
>> right? Before that, if the first init failed, you could trigger a
>> re-init via ioctl(), and presumably trigger the race sashiko is
>> complaining about by spamming ioctl() + sysfs writes on separate
>> threads.

Yes, this is the race I had in mind and probably what sashiko complained about
in it's review too. I missed this patch from earlier. This should avoid any
racing.

>>
>>>> t1                                 | t2
>>>> ---------------------------------- | ----------------------------------
>>>> sev_firmware_shutdown()            | sev_platform_init()
>>>>   unregister_verify_mitigation()   |   register_verify_mitigation()
>>>>     sysfs_remove_group()           |     sysfs_create_group()
>>>>
>>>> Both sides touch sev->verify_mit without serialization. The same race also
>>>> exists for init vs init which is no longer covered by sev_cmd_mutex once
>>>> register moves outside it.
>>>
>>> I don't think you can have init vs init race, can you? This just all seems
>>> odd to me. Have you created all these race scenarios to test this out?
>>>
>>> Would putting the regsiter/unregister under the sev_cmd_mutex and then
>>> taking the sev_cmd_mutex upon entry to _show()/_store() fix all this?
>>> After obtaining the mutex in _show()/_store(), you check for
>>> sev->verify_mit and return an error if NULL. Then you can use the
>>> __sev_do_cmd_locked() to issue any commands.
>>
>> As long as sysfs_remove_group() happens before
>> __sev_firmware_shutdown() it seems like it should be fine since sysfs
>> will do its own synchronization. IIUC we might not need this locking
>> at all assuming the above is applied?
> 
> That's what I'm thinking. I'll let Pratik confirm.
> 

Yes, sysfs should do its own synchronization and I'm assuming this means that we
don't need any locks anymore and I can get rid of the sev_mit_sysfs_mutex and
move unregister outside the sev_cmd_mutex.

I tested this with putting a msleep() in the _show()/_store() and in parallel
rmmod calling shutdown. This seems to work without issues whereas with the
former approach I could deadlock waiting on sev_cmd_mutex.

> 
>>> Also, on the register function, all you need is the check for
>>> !(sev->snp_feat_info_0.ecx & SNP_VERIFY_MITIGATION_SUPPORTED) since if
>>> !sev->snp_plat_status.feature_info is true, so is this this check. And, as
>>> the spec says, the required firmware state is based on the mitigation
>>> requirements, so I don't think you should be checking for snp_initialized.

Ack, will just keep the SNP_VERIFY_MITIGATION_SUPPORTED check in the next
iteration.

Thanks Tom and Tycho!
--Pratik

> Thanks,
> Tom
> 
>>
>> Tycho
>