| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e98582db-39d6-4a09-8f9c-88b0aa0c4676@amd.com>
Date: Tue, 2 Dec 2025 16:39:50 -0600
From: "Kalra, Ashish" <ashish.kalra@....com>
To: Alexey Kardashevskiy <aik@....com>, Tom Lendacky
<thomas.lendacky@....com>, linux-kernel@...r.kernel.org
Cc: linux-crypto@...r.kernel.org, John Allen <john.allen@....com>,
Herbert Xu <herbert@...dor.apana.org.au>,
"David S. Miller" <davem@...emloft.net>, Joerg Roedel <joro@...tes.org>,
Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
Borislav Petkov <bp@...e.de>, "Borislav Petkov (AMD)" <bp@...en8.de>,
Dan Williams <dan.j.williams@...el.com>, Jason Gunthorpe <jgg@...pe.ca>,
Jerry Snitselaar <jsnitsel@...hat.com>, Vasant Hegde <vasant.hegde@....com>,
Gao Shiyuan <gaoshiyuan@...du.com>, Sean Christopherson <seanjc@...gle.com>,
Kim Phillips <kim.phillips@....com>, Nikunj A Dadhania <nikunj@....com>,
Michael Roth <michael.roth@....com>, Paolo Bonzini <pbonzini@...hat.com>,
iommu@...ts.linux.dev, x86@...nel.org, linux-coco@...ts.linux.dev
Subject: Re: [PATCH kernel v3 4/4] crypto/ccp: Implement SEV-TIO PCIe IDE
(phase1)
On 12/2/2025 4:30 PM, Alexey Kardashevskiy wrote:
>
>
> On 3/12/25 01:52, Tom Lendacky wrote:
>> On 12/1/25 20:44, Alexey Kardashevskiy wrote:
>>> Implement the SEV-TIO (Trusted I/O) firmware interface for PCIe TDISP
>>> (Trust Domain In-Socket Protocol). This enables secure communication
>>> between trusted domains and PCIe devices through the PSP (Platform
>>> Security Processor).
>>>
>>> The implementation includes:
>>> - Device Security Manager (DSM) operations for establishing secure links
>>> - SPDM (Security Protocol and Data Model) over DOE (Data Object Exchange)
>>> - IDE (Integrity Data Encryption) stream management for secure PCIe
>>>
>>> This module bridges the SEV firmware stack with the generic PCIe TSM
>>> framework.
>>>
>>> This is phase1 as described in Documentation/driver-api/pci/tsm.rst.
>>>
>>> On AMD SEV, the AMD PSP firmware acts as TSM (manages the security/trust).
>>> The CCP driver provides the interface to it and registers in the TSM
>>> subsystem.
>>>
>>> Detect the PSP support (reported via FEATURE_INFO + SNP_PLATFORM_STATUS)
>>> and enable SEV-TIO in the SNP_INIT_EX call if the hardware supports TIO.
>>>
>>> Implement SEV TIO PSP command wrappers in sev-dev-tio.c and store
>>> the data in the SEV-TIO-specific structs.
>>>
>>> Implement TSM hooks and IDE setup in sev-dev-tsm.c.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@....com>
>>
>> Just some minor comments below. After those are addressed:
>>
>> For the ccp related changes in the whole series:
>>
>> Acked-by: Tom Lendacky <thomas.lendacky@....com>
>
> Dan did it right (thanks Dan!).
>
>
>>
>>> ---
>>> Changes:
>>> v2:
>>> * moved declarations from sev-dev-tio.h to sev-dev.h
>>> * removed include "sev-dev-tio.h" from sev-dev.c to fight errors when TSM is disabled
>>> * converted /** to /* as these are part of any external API and trigger unwanted kerneldoc warnings
>>> * got rid of ifdefs
>>> * "select PCI_TSM" moved under CRYPTO_DEV_SP_PSP
>>> * open coded SNP_SEV_TIO_SUPPORTED
>>> * renamed tio_present to tio_supp to match the flag name
>>> * merged "crypto: ccp: Enable SEV-TIO feature in the PSP when supported" to this one
>>> ---
>>> drivers/crypto/ccp/Kconfig | 1 +
>>> drivers/crypto/ccp/Makefile | 4 +
>>> drivers/crypto/ccp/sev-dev-tio.h | 123 +++
>>> drivers/crypto/ccp/sev-dev.h | 9 +
>>> include/linux/psp-sev.h | 11 +-
>>> drivers/crypto/ccp/sev-dev-tio.c | 864 ++++++++++++++++++++
>>> drivers/crypto/ccp/sev-dev-tsm.c | 405 +++++++++
>>> drivers/crypto/ccp/sev-dev.c | 51 +-
>>> 8 files changed, 1465 insertions(+), 3 deletions(-)
>>>
>>
>>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>>> index 9e0c16b36f9c..d6095d1467b3 100644
>>> --- a/drivers/crypto/ccp/sev-dev.c
>>> +++ b/drivers/crypto/ccp/sev-dev.c
>>> @@ -75,6 +75,10 @@ static bool psp_init_on_probe = true;
>>> module_param(psp_init_on_probe, bool, 0444);
>>> MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be initialized on module init. Else the PSP will be initialized on the first command requiring it");
>>> +static bool sev_tio_enabled = IS_ENABLED(CONFIG_PCI_TSM);
>>> +module_param_named(tio, sev_tio_enabled, bool, 0444);
>>> +MODULE_PARM_DESC(tio, "Enables TIO in SNP_INIT_EX");
>>
>> Hmmm... I thought you said you wanted to hide the module parameter if
>> CONFIG_PCI_TSM isn't enabled. Either way, it's fine.
>
> I did but you did not and I do not care that much :)
>
>>
>>> +
>>> MODULE_FIRMWARE("amd/amd_sev_fam17h_model0xh.sbin"); /* 1st gen EPYC */
>>> MODULE_FIRMWARE("amd/amd_sev_fam17h_model3xh.sbin"); /* 2nd gen EPYC */
>>> MODULE_FIRMWARE("amd/amd_sev_fam19h_model0xh.sbin"); /* 3rd gen EPYC */
>>> @@ -251,7 +255,7 @@ static int sev_cmd_buffer_len(int cmd)
>>> case SEV_CMD_SNP_COMMIT: return sizeof(struct sev_data_snp_commit);
>>> case SEV_CMD_SNP_FEATURE_INFO: return sizeof(struct sev_data_snp_feature_info);
>>> case SEV_CMD_SNP_VLEK_LOAD: return sizeof(struct sev_user_data_snp_vlek_load);
>>> - default: return 0;
>>> + default: return sev_tio_cmd_buffer_len(cmd);
>>> }
>>> return 0;
>>> @@ -1434,6 +1438,19 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>>> data.init_rmp = 1;
>>> data.list_paddr_en = 1;
>>> data.list_paddr = __psp_pa(snp_range_list);
>>> +
>>> + bool tio_supp = !!(sev->snp_feat_info_0.ebx & SNP_SEV_TIO_SUPPORTED);
>>
>> Please put the variable definition at the top of the "if" block instead
>> of in the middle of the code.
>>> +
>>> + data.tio_en = tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();
>>
>> Don't you still want to take CONFIG_PCI_TSM into account?
>>
>> data.tio_en = IS_ENABLED(CONFIG_PCI_TSM) && tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();
>>
>> or
>> if (IS_ENABLED(CONFIG_PCI_TSM)
>> data.tio_en = tio_supp && sev_tio_enabled && amd_iommu_sev_tio_supported();
>>
>> But if you change back to #ifdef the module parameter, then you won't
>> need the IS_ENABLED() check here because sev_tio_enabled will be set
>> based on CONFIG_PCI_TSM and will be false and not changeable if
>> CONFIG_PCI_TSM is not y.
>
>
> Ah true. I thought sev_tio_enabled=IS_ENABLED(CONFIG_PCI_TSM) does it but missed that sev_tio_enabled is exported as a parameter so not a constant at compile time.
>
>
>>> +
>>> + /*
>>> + * When psp_init_on_probe is disabled, the userspace calling
>>> + * SEV ioctl can inadvertently shut down SNP and SEV-TIO causing
>>> + * unexpected state loss.
>>> + */
>>
>> After this is merged, lets see if sev_move_to_init_state() can be
>> cleaned up to avoid this situation.
>
> Do we want to keep psp_init_on_probe, why? Thanks,
>
I think we still need to support "psp_init_on_probe" to support deferred SEV initialization and continue supporting SEV INIT_EX.
Thanks,
Ashish
>
>>
>> Thanks,
>> Tom
>>
>>> + if (data.tio_en && !psp_init_on_probe)
>>> + dev_warn(sev->dev, "SEV-TIO as incompatible with psp_init_on_probe=0\n");
>>> +
>>> cmd = SEV_CMD_SNP_INIT_EX;
>>> } else {
>>> cmd = SEV_CMD_SNP_INIT;
>>> @@ -1471,7 +1488,8 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>>> snp_hv_fixed_pages_state_update(sev, HV_FIXED);
>>> sev->snp_initialized = true;
>>> - dev_dbg(sev->dev, "SEV-SNP firmware initialized\n");
>>> + dev_dbg(sev->dev, "SEV-SNP firmware initialized, SEV-TIO is %s\n",
>>> + data.tio_en ? "enabled" : "disabled");
>>> dev_info(sev->dev, "SEV-SNP API:%d.%d build:%d\n", sev->api_major,
>>> sev->api_minor, sev->build);
>>> @@ -1479,6 +1497,23 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
>>> atomic_notifier_chain_register(&panic_notifier_list,
>>> &snp_panic_notifier);
>>> + if (data.tio_en) {
>>> + /*
>>> + * This executes with the sev_cmd_mutex held so down the stack
>>> + * snp_reclaim_pages(locked=false) might be needed (which is extremely
>>> + * unlikely) but will cause a deadlock.
>>> + * Instead of exporting __snp_alloc_firmware_pages(), allocate a page
>>> + * for this one call here.
>>> + */
>>> + void *tio_status = page_address(__snp_alloc_firmware_pages(
>>> + GFP_KERNEL_ACCOUNT | __GFP_ZERO, 0, true));
>>> +
>>> + if (tio_status) {
>>> + sev_tsm_init_locked(sev, tio_status);
>>> + __snp_free_firmware_pages(virt_to_page(tio_status), 0, true);
>>> + }
>>> + }
>>> +
>>> sev_es_tmr_size = SNP_TMR_SIZE;
>>> return 0;
>>> @@ -2758,8 +2793,20 @@ static void __sev_firmware_shutdown(struct sev_device *sev, bool panic)
>>> static void sev_firmware_shutdown(struct sev_device *sev)
>>> {
>>> + /*
>>> + * Calling without sev_cmd_mutex held as TSM will likely try disconnecting
>>> + * IDE and this ends up calling sev_do_cmd() which locks sev_cmd_mutex.
>>> + */
>>> + if (sev->tio_status)
>>> + sev_tsm_uninit(sev);
>>> +
>>> mutex_lock(&sev_cmd_mutex);
>>> +
>>> __sev_firmware_shutdown(sev, false);
>>> +
>>> + kfree(sev->tio_status);
>>> + sev->tio_status = NULL;
>>> +
>>> mutex_unlock(&sev_cmd_mutex);
>>> }
>>>
>>
>
Powered by blists - more mailing lists