Re: [RFC PATCH v1 09/11] crypto/ccp: Defer SEV-TIO link setup until evidence acceptance

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

Alexey Kardashevskiy <aik@amd.com> writes:

> On 29/10/26 02:26, Aneesh Kumar K.V (Arm) wrote:
>> AMD SEV-TIO currently establishes its SPDM session and enables IDE during
>> connect, before userspace can retrieve or accept the device identity.
>> Move AMD link setup behind its evidence_accept callback.

 [ ... 49 lines skipped ... ] 

>>   	dev_data->req.sla = 0;
>>   	dev_data->resp.sla = 0;
>>   	dev_data->scratch.sla = 0;
>>   	dev_data->respbuf = NULL;
>>   	dev_data->reqbuf = NULL;
>> -	sla_free(dev_data->output, tio_status->spdm_out_size_max, true);
>> +	sla_free(dev_data->output, dev_data->output_len, true);
>> +	dev_data->output = SLA_NULL;
>
> This is a bugfix (thanks for pointing!) and does not belong here. Want to repost, or I can (and should, really)?
>
>>   }
>>   
>>   static int spdm_ctrl_alloc(struct tsm_dsm_tio *dev_data)
>> @@ -674,7 +672,7 @@ static int spdm_ctrl_alloc(struct tsm_dsm_tio *dev_data)
>>   	dev_data->output = sla_alloc(dev_data->output_len, true);
>>   
>>   	if (IS_SLA_NULL(dev_data->req) || IS_SLA_NULL(dev_data->resp) ||
>> -	    IS_SLA_NULL(dev_data->scratch) || IS_SLA_NULL(dev_data->dev_ctx)) {
>> +	    IS_SLA_NULL(dev_data->scratch) || IS_SLA_NULL(dev_data->output)) {
>
> the same.
>
>>   		ret = -ENOMEM;
>>   		goto free_spdm_exit;
>>   	}
>> @@ -773,7 +771,8 @@ int sev_tio_dev_create(struct tsm_dsm_tio *dev_data, u16 device_id,
>>   free_data_pg_exit:
>>   	snp_free_firmware_page(data_pg);
>>   free_ctx_exit:
>> -	sla_free(create.dev_ctx_sla, tio_status->devctx_size, true);
>> +	sla_free(dev_data->dev_ctx, tio_status->devctx_size, true);
>
> and this. Is this codex? :)
>

Yes. 

>
>> +	dev_data->dev_ctx = SLA_NULL;
>>   	return ret;
>>   }
>>   

 [ ... 179 lines skipped ... ] 

>> +
>> +	/* Reserve TC0's stream ID without programming or enabling IDE. */
>> +	memset(ids, 0xff, sizeof(ids));
>> +	ids[0] = dev_data->ide[0]->stream_id;
>> +	dev_data->cert_slot = 0;
>> +	ret = sev_tio_dev_connect(dev_data, BIT(0), ids, dev_data->cert_slot);
>> +	ret = dsm_collect_identity(dsm, ret);
>
> Cannot librspdm do that collection, directly via DOE? Ideally in the
> generic code? There nothing special for the PSP to do and stopping
> SEV-TIO's connect half way is weird.
>


This patch does not use rust-spdm and avoids introducing that
dependency. It uses slot 0 to initiate the session, retrieve the
certificate chain, and return it to the host. After host approval, it
resumes the session and enables IDE. Only slot 0 is supported, and the
host can reject the device identity before IDE is enabled.

>
>
>> +	if (!ret)
>> +		return 0;
>> +
>> +disconnect:
>> +	dsm_disconnect(pdev);
>> +	return ret;
>> +}
>>   
>> -	tc_mask = streams_setup(dev_data->ide, ids);
>> +static int dsm_evidence_accept(struct pci_dev *pdev)
>> +{
>> +	struct tio_dsm *dsm = pdev_to_tio_dsm(pdev);
>> +	struct tsm_dsm_tio *dev_data = &dsm->data;
>> +	u8 ids[TIO_IDE_MAX_TC];
>> +	int ret;
>>   
>> -	dev_data->cert_slot = pdev->tsm->evidence.slot;
>> -	ret = sev_tio_dev_connect(dev_data, tc_mask, ids, dev_data->cert_slot);
>> -	ret = sev_tio_spdm_cmd(dsm, ret);
>> -	if (ret)
>> -		goto free_exit;
>> +	if (pdev->tsm->evidence.slot)
>> +		return -EOPNOTSUPP;
>> +	if (!dsm->pending_spdm)
>> +		return -ENODATA;
>>   
>> -	streams_enable(dev_data->ide);
>> +	streams_setup(dev_data->ide, ids);
>
>
> I kinda get the appeal of not enabling IDE before accepting but in
> practice the host still has to have a knob in IOMMU to prevent a
> malicious device from DMAing T=1 trafic so accept() would trigger
> enable_dma() hook.
>

This enables IDE; it is not the TDISP RUN transition. Am I missing
something here?

>
>
>> +	ret = sev_tio_spdm_cmd(dsm, dsm->pending_spdm);
>> +	dsm->pending_spdm = 0;
>> +	if (ret)
>> +		goto err_session;
>> +	dsm->session_connected = true;
>>   
>> +	ret = streams_enable(dev_data->ide);
>> +	if (ret)
>> +		goto err_session;
>>   	ret = streams_register(dev_data->ide);
>>   	if (ret)
>> -		goto free_exit;
>> -
>> +		goto err_session;
>>   	return 0;
>>   
>> -free_exit:
>> -	sev_tio_dev_reclaim(dev_data);
>> -
>> -	streams_disable(dev_data->ide);
>> -ide_free_exit:
>> -
>> -	streams_teardown(dev_data->ide);
>> -
>> +err_session:
>> +	/* Disconnect owns cleanup after firmware or IDE setup has started. */
>> +	pdev->tsm->error = true;
>>   	return ret;
>>   }
>>   
>> @@ -339,14 +468,20 @@ static void dsm_disconnect(struct pci_dev *pdev)
>>   	struct tsm_dsm_tio *dev_data = &dsm->data;
>>   	int ret;
>>   
>> -	ret = sev_tio_dev_disconnect(dev_data, force);
>> -	ret = sev_tio_spdm_cmd(dsm, ret);
>> -	if (ret && !force) {
>> -		ret = sev_tio_dev_disconnect(dev_data, true);
>> -		sev_tio_spdm_cmd(dsm, ret);
>> +	/* A paused or failed connect needs forced firmware cleanup. */
>> +	if (dev_data->reqbuf && dev_data->respbuf) {
>> +		force |= !dsm->session_connected;
>> +		ret = sev_tio_dev_disconnect(dev_data, force);
>> +		ret = sev_tio_spdm_cmd(dsm, ret);
>> +		if (ret && !force) {
>> +			ret = sev_tio_dev_disconnect(dev_data, true);
>> +			sev_tio_spdm_cmd(dsm, ret);
>> +		}
>>   	}
>>   
>>   	sev_tio_dev_reclaim(dev_data);
>> +	dsm->session_connected = false;
>> +	dsm->pending_spdm = 0;
>>   
>>   	streams_disable(dev_data->ide);
>>   	streams_unregister(dev_data->ide);
>> @@ -357,6 +492,8 @@ static struct pci_tsm_ops sev_tsm_ops = {
>>   	.probe = dsm_probe,
>>   	.remove = dsm_remove,
>>   	.connect = dsm_connect,
>> +	.evidence_accept = dsm_evidence_accept,
>
> I saw in your branch you are ditching the connect() hook, is that the plan? Thanks,
>


Yes. In the full branch, once rust-spdm is integrated, this entire flow
moves to the SPDM accept/evidence_accept phase.

>
>
>> +	.evidence_types = BIT(PCI_TSM_EVIDENCE_TYPE_CERT0),
>>   	.disconnect = dsm_disconnect,
>>   };
>>   
>

-aneesh