Message-ID: <fd62ecf6-631f-f55f-dc1c-827008aa0e56@amd.com>
Date:   Wed, 22 Feb 2023 10:50:12 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH RFC v8 27/56] crypto: ccp: Add the
 SNP_{SET,GET}_EXT_CONFIG command
Content-Language: en-US
To:     Zhi Wang <zhi.wang.linux@gmail.com>,
        Michael Roth <michael.roth@amd.com>
Cc:     kvm@vger.kernel.org, linux-coco@lists.linux.dev,
        linux-mm@kvack.org, linux-crypto@vger.kernel.org, x86@kernel.org,
        linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com,
        jroedel@suse.de, hpa@zytor.com, ardb@kernel.org,
        pbonzini@redhat.com, seanjc@google.com, vkuznets@redhat.com,
        jmattson@google.com, luto@kernel.org, dave.hansen@linux.intel.com,
        slp@redhat.com, pgonda@google.com, peterz@infradead.org,
        srinivas.pandruvada@linux.intel.com, rientjes@google.com,
        dovmurik@linux.ibm.com, tobin@ibm.com, bp@alien8.de,
        vbabka@suse.cz, kirill@shutemov.name, ak@linux.intel.com,
        tony.luck@intel.com, marcorr@google.com,
        sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
        dgilbert@redhat.com, jarkko@kernel.org, ashish.kalra@amd.com,
        nikunj.dadhania@amd.com, Brijesh Singh <brijesh.singh@amd.com>
References: <20230220183847.59159-1-michael.roth@amd.com>
 <20230220183847.59159-28-michael.roth@amd.com>
 <20230222143205.00007635@gmail.com>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20230222143205.00007635@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Qm1Da0lHclFQMWFUUUZ3QUQ3N2VTNXhPVWRHVEZjdFJKOEFaanRNV2VDSzZu?=
 =?utf-8?B?VDlHWjdXZitDS0ZlUDVLMTBXNTk5S3g5bHVzSWliNm5sOTlETmJPZVVHYzFa?=
 =?utf-8?B?SWRkd1NPT0Q2UStsOThpZUFPZHhmdnU5Y2FPRDdKS0JCM3c5MEEwbER4UGhD?=
 =?utf-8?B?Z3VMRCszUHJ0UGZWditzRk9CaTRBVVUvaXFObmduSlczTmxaYWNWcEdOY1dT?=
 =?utf-8?B?NFF2SUNqZjhtNFlWQW1USzJIbVUzZGp5aTYzd0xHVUR1VlFYblE5VW1TOFdv?=
 =?utf-8?B?MXNkYk1tMWxzQU1qOFM5cjVwbFd6QjZmNmUvWnk0UHBNdE5VU0lMK1ZjaW9P?=
 =?utf-8?B?eFpWZ1JUUURaaVB5eVFuMHhYWkh2Y0I5MDUxQm9PME9VV2RrOVBwT1RIT2NI?=
 =?utf-8?B?VTA1NmJFRG1vaU8rdGlyVzI2dlprQ3pCQlZQSmhYYjY0UUJ2dkZ1VFpMTnJV?=
 =?utf-8?B?amJabjVmUDBiWC9FSzVFdEVtUDJaUzROM205WEJMUXNMTXd3NVZCejZsVVZW?=
 =?utf-8?B?dWI5cC95QlpIREV5eVVTekcrZ2E0eFUwblc2YWZoNU5OOHZXQ0ZjMncvUnhR?=
 =?utf-8?B?blN3QXVUT3RoMDNMZG9aL0IvaGFnWEpSbW5aMENvVGR4RnJiU2lscElWb080?=
 =?utf-8?B?NFdMbEMyOTJpQ1F1T2g0RklhOHBxQzNxVGt3N1UrL0JCdXVIRDFLMm1uVjRi?=
 =?utf-8?B?Q2I2ZDlyU01tMlJsVzJUbG54SzFER29hWC91QklyU3N1bGtJV0J4VjFra1JO?=
 =?utf-8?B?M0JERnZtRFhwU2ZkN09aY0FtbEFHVXRZRHlpMDJQc2ZsaW5FckVFWGExWUZk?=
 =?utf-8?B?bWY0cW1NME0xZ09kQjcvYnZkR2VaUVQrc1BLbThjangzY2xyMjlMc3B1eDMy?=
 =?utf-8?B?eDUwdTRzQ1RFd2dXcnIrRDVFWUxjckkxTHVxSjhiaHlpeW1GQ2dEc01CbWJY?=
 =?utf-8?B?eFFoRXNKdkcvaGtSc1NCU2lNcnl6V1NLYzlvdlVkV1FBdlVHSzREVHJQYXJy?=
 =?utf-8?B?amdpZDJ5UXZVK1ZHKzdYSVRtWnB4Ni9IWG5uU1pLVklXZ0F3WGVJcmhHWUdz?=
 =?utf-8?B?VHdwTmxveGtmMTZ2dkpITUQvRG5CVEFjd1dJcmZTNEYrYU9LTDZjL2taMVlL?=
 =?utf-8?B?UkU1eHJJbzkxOUdyY3IvQXcxTDR5K1FYS1llVUlBanVJdG00dEdVZEhObXdm?=
 =?utf-8?B?K25Cdmx6ZGI4ZzNxTFN0ZkVqNmNUcjdXUXhGT05ibHNHTkdwSjBUQkYrRG1Y?=
 =?utf-8?B?Rldzd1ZwMzJlRWVYdmJGaFlKT3hYWElyS3V6ZWszOVNjeTlLUFVkbHI5NDRv?=
 =?utf-8?B?aEExcUd2Yzhrd3NpVjRqVDJaRXpLaCtCZ1h2N2JpY3dqUytjdlh0QXhjV2l5?=
 =?utf-8?B?QjN1Mkd3ckQ4VWhSdVFKMEdROVVyR1Rwa3JlcXV2K1ZBT3pJRCtSN29IY1pM?=
 =?utf-8?B?NS9zQVdnc3Z6MGNqMEkyRmx2bmRmejlXMTVmZEk5ZG1Wc0V4WVUrTTlSN1ZW?=
 =?utf-8?B?ck80TWk2RUtPYkQ2NnFTU0J4OVlmWUlCcU1ISE8xZ2VwVyt5TElKcHQ2bmVY?=
 =?utf-8?B?TFRabEtSTHg2KzNUNmNaUW9PanpaNHdvKzlVN0FrQmRZM3RDTDdoWFVra1k5?=
 =?utf-8?B?K045NWFnZ1BPMTZROU55bHhCU0YzTTA0UGZvdXZHaEs3MXpMTkprK3AxV09i?=
 =?utf-8?B?di9vRW9weDk3UjRQUkNwalhKMUNLTVVYU2haUkd0ZzFBdkRpc2R3czBxeTY3?=
 =?utf-8?B?eFdmNzZOTDFlc014alpHYmZLSldDZ3J6ak5jTnBLYjBkUjd5bTdhNCtRYmRL?=
 =?utf-8?B?OEhmWG5JZFJYOU1YUno0WDE2MGdTbkdicVVEMTd2R0RuT0MxM3NqUTRMV1M0?=
 =?utf-8?B?MDNEbXRENy9zWjB5K0JFc0dLSCtGNkdzUHFoSWRnRHpTSmdsRHdIK2RMNWk2?=
 =?utf-8?B?dnUwM3NQOU1QUG9JYUYvSmFoS0VsOFdPTnhsVVdyWGhlVVp4R1VqaXBiQlF3?=
 =?utf-8?B?bmhtRXRFcm5RZEV3d2FLTStlRjRWU0pFcUJaaXNrbU5xeGt4R1JRK2RXdjkx?=
 =?utf-8?B?TWFCWmRXcXRpOXRaMXgwbWZRdldPQmp0Rk9qUlJzbkVSWGV3OFFNd3A4bWYv?=
 =?utf-8?Q?t9zv5AkavW4GInqLhIU0GI2nh?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 68ea34e7-8d7c-4468-99cb-08db14f4df90
X-MS-Exchange-CrossTenant-AuthSource: DM4PR12MB5229.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2023 16:50:16.1396
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: uU/wfA0fHutL2F7/e5TYNXDFzG+7ISyjQdFSX20AsP8L51ip2ozFMffiR2ByJ90one0Raj326sLD6X1sGMR2/w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR12MB5400
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 2/22/23 06:32, Zhi Wang wrote:
> On Mon, 20 Feb 2023 12:38:18 -0600
> Michael Roth <michael.roth@amd.com> wrote:
> 
>> From: Brijesh Singh <brijesh.singh@amd.com>
>>
>> The SEV-SNP firmware provides the SNP_CONFIG command used to set the
>> system-wide configuration value for SNP guests. The information includes
>> the TCB version string to be reported in guest attestation reports.
>>
>> Version 2 of the GHCB specification adds an NAE (SNP extended guest
>> request) that a guest can use to query the reports that include additional
>> certificates.
>>
>> In both cases, userspace provided additional data is included in the
>> attestation reports. The userspace will use the SNP_SET_EXT_CONFIG
>> command to give the certificate blob and the reported TCB version string
>> at once. Note that the specification defines certificate blob with a
>> specific GUID format; the userspace is responsible for building the
>> proper certificate blob. The ioctl treats it an opaque blob.
>>
>> While it is not defined in the spec, but let's add SNP_GET_EXT_CONFIG
>> command that can be used to obtain the data programmed through the
>> SNP_SET_EXT_CONFIG.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> Signed-off-by: Michael Roth <michael.roth@amd.com>
>> ---
>>   Documentation/virt/coco/sev-guest.rst |  27 ++++++
>>   drivers/crypto/ccp/sev-dev.c          | 123 ++++++++++++++++++++++++++
>>   drivers/crypto/ccp/sev-dev.h          |   4 +
>>   include/uapi/linux/psp-sev.h          |  17 ++++
>>   4 files changed, 171 insertions(+)
>>

>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 65e13a562f3b..b56b00ca2cd4 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1481,6 +1481,10 @@ static int __sev_snp_shutdown_locked(int *error)
>>   	data.length = sizeof(data);
>>   	data.iommu_snp_shutdown = 1;
>>   
>> +	/* Free the memory used for caching the certificate data */
>> +	kfree(sev->snp_certs_data);
>> +	sev->snp_certs_data = NULL;
>> +
>>   	wbinvd_on_all_cpus();
>>   
>>   retry:
>> @@ -1793,6 +1797,118 @@ static int sev_ioctl_snp_platform_status(struct sev_issue_cmd *argp)
>>   	return ret;
>>   }
>>   
>> +static int sev_ioctl_snp_get_config(struct sev_issue_cmd *argp)
>> +{
>> +	struct sev_device *sev = psp_master->sev_data;
>> +	struct sev_user_data_ext_snp_config input;
>> +	int ret;
>> +
>> +	if (!sev->snp_initialized || !argp->data)
>> +		return -EINVAL;
>> +
>> +	memset(&input, 0, sizeof(input));
>> +
>> +	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>> +		return -EFAULT;
>> +
>> +	/* Copy the TCB version programmed through the SET_CONFIG to userspace */
>> +	if (input.config_address) {
>> +		if (copy_to_user((void * __user)input.config_address,
>> +				 &sev->snp_config, sizeof(struct sev_user_data_snp_config)))
>> +			return -EFAULT;
>> +	}
>> +
>> +	/* Copy the extended certs programmed through the SNP_SET_CONFIG */
>> +	if (input.certs_address && sev->snp_certs_data) {
>> +		if (input.certs_len < sev->snp_certs_len) {
>> +			/* Return the certs length to userspace */
>> +			input.certs_len = sev->snp_certs_len;
>> +
>> +			ret = -ENOSR;

We should be consistent with the other SEV ioctls that return required 
lengths and return -EIO here instead -ENOSR.

Thanks,
Tom

>> +			goto e_done;
>> +		}
>> +
> 
> What about if input.certs_len > sev->snp_certs_len? Is it possbile for the
> userspace to know the length of data in the buffer? (I guess it might be able
> to know the certs len through the blob data, but a comment here would be nice)
> 
>> +		if (copy_to_user((void * __user)input.certs_address,
>> +				 sev->snp_certs_data, sev->snp_certs_len))
>> +			return -EFAULT;
>> +	}
>> +
>> +	ret = 0;
>> +
>> +e_done:
>> +	if (copy_to_user((void __user *)argp->data, &input, sizeof(input)))
>> +		ret = -EFAULT;
>> +
>> +	return ret;
>> +}