Message-ID: <c548c555-279e-4ec4-d51f-7f8cdd501010@amd.com>
Date:   Wed, 22 Feb 2023 16:43:54 -0600
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.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, thomas.lendacky@amd.com, 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, 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:   "Kalra, Ashish" <ashish.kalra@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?bXNRU0xGRU1sRFRXVmFVNTBYaFg5VWJxeWp0TDZ2REtiaFpYRVBLYUZ5dHFV?=
 =?utf-8?B?MnR2TzhZV0VlTE5yQ0d5SnFMb243NlBwbW1EUzJBYkFSdUNPUnRoNEhGamFF?=
 =?utf-8?B?MEhBMnAyL0VHdTVheDRaRTNWWHJyd3YzcWdqRFoyNzI5YTVHczh5RG5NbDZJ?=
 =?utf-8?B?eWpoQ09memJpdDRadjUyUlRBcklrNmhoQzViWXVmL0E1bzc2NDVGYTBXMGJV?=
 =?utf-8?B?Zjc5UDJvNnRycnE2T1JYV1J6MkhHdUhMRllhanlSMlcxT1ZFT3k1TU9ZaXBG?=
 =?utf-8?B?RXFTQVpFSEh4eGJ1WHJ2emhDbktuTnNGeXdwZ3N2Z0hNUXluMnR3QXJmREdF?=
 =?utf-8?B?VnVEbWlTaXRjTmR4bzVLNGltUU9ReldSdXg3NENrSzJOY3YxT0hXSzhnc1Vp?=
 =?utf-8?B?WVdaZFF2K0NDUmUyK3pnS0pVWUpMaWthRHdhd2FyMjlObVhUNW4yMEs2MW9o?=
 =?utf-8?B?VzEyRnlOZmNjdXFlblZyS01zUDllWGkwbUhEV2l2ci9ZM1prc2o3MVhpajBL?=
 =?utf-8?B?V0R3OWJIalRXYWIvK2ppb3JTZ1JZTjhwTjM1MW1OZHEzVzNnQmdhNnlYTjlm?=
 =?utf-8?B?RDhmU1B0WktDWWRyZ1JML2pDMFpBVWFaSEpzSUdYaEZTU1FKTVZ0bFY4Qm5s?=
 =?utf-8?B?RHdoRE1hKzVoVnVJd3gxVlVuekN1a0pvZVZoZkQzR0U4ckVnU1NjWTB0dVNE?=
 =?utf-8?B?VzFQeWxvVkRvYzJoMXQ5ZHRtcndIR1hBNEFaZXArUmNVQ1UzTXFGOFJZNzNE?=
 =?utf-8?B?SEFkWCtxUmQzY0wxMENaWERDbEZkWXYwT29hbUdtSktETHEwdUFzQzlBSXFv?=
 =?utf-8?B?M0xSNUZ3VzJaZE9uRElRYUd4WS9jKzRSWVZOTjZuL1E5WVNjREJLOWhlNVI4?=
 =?utf-8?B?SHExa25qVDJINFZVTi9XWnNOZ25aTldWOStBVWtiYjBTR1VnaUFtbjZyQW1Y?=
 =?utf-8?B?Z3N6ZU1wellxNHZ6aDluWWdxczhkV0lZK0xTb1NYOEhybThrTlNMUHM1WU0v?=
 =?utf-8?B?djVQRGJTakVqNCtXYSswK3Nra25uMGloOUQ5bFBiOVd6L0NEWEE4WXZWcTkx?=
 =?utf-8?B?VWtIWnZYeXhoUmtHQTZVZWhLb0lUd2VONmZYY2tCOGVwYUt0OW5hMUtMQ3Nt?=
 =?utf-8?B?bDd4UXFFSjFtL3FFT0YvTXB5N1FiNUM3MXlwdDV1RlI2dVIwaXk1V05qcmJl?=
 =?utf-8?B?YmdvK1d5Z2c1U3pNS1RJRzdNVmE5am5MdGo5b2lScWlaOHMzUzlaY3l1Yk1w?=
 =?utf-8?B?SzU4RGF2akloanpHMTUwbWFTSlBaeml4Wk5XVmM2bSsxWWVQNm1Ubkl3N3ly?=
 =?utf-8?B?ZGNOWU9zL1hjYlhNWDZreUJkRDRCUG1YazVYSW43SWw0TUVxR252U1VHeFd1?=
 =?utf-8?B?OVY5UVo4UzMwT1pnT0hocWJOdVhTelVaOXYzTnA1WlNJSFk1SXlwY2E4Vldw?=
 =?utf-8?B?cFgwSUJvWVR6L0kzajVXdnhEd0ZEY0YweE01VE9Fd0NGaUFkSGNyLzAxZVB4?=
 =?utf-8?B?TjJ4Y21FL1VyakcybUs0NENKUTMvZ3pPcDV4RFVGb1RQT2VrQzdya29IVDVR?=
 =?utf-8?B?UzNxL3pEVDJrU3JwZTkxODFWSjdvMkNQK0xUUnBFQW5sSmtKejRwUmU2YURJ?=
 =?utf-8?B?UzRUZTZNNThkZFVZSWhUREIzdGRPZnVzN2Y1WWVYMzRFQ2p2bmI2bHNGVmxE?=
 =?utf-8?B?N25wZ0hWWElodTNyQUpQY3U3cElDUksvOTAwZy9tSzYwaXRiczFpdHk2ZlhG?=
 =?utf-8?B?dlpNcFdYZ3dvWklOMy9FR2UyU3FGTnBYbFQ2YVBwQjBudFhpdnV2SkNkd3ZH?=
 =?utf-8?B?aWc4S3d3THN1ZVZ4bFBXU3lGWE9iZnlZRXlSYmZsUkFvR1VISVNWK1g2OVRm?=
 =?utf-8?B?MzVldEtsVDNOSS9sM0NGcVplQXBKcy95d2wzazFnRURFdjF6cWE4YUF0aG1i?=
 =?utf-8?B?KzNRTUlqdFExMWtGUzREeUNGLzd0R3BCTHRqODFaNzJJUjhyWG1FNEdHcThj?=
 =?utf-8?B?T3QxaFhKYXlYVGFmM3FYVlNXbVc1VnNleWJCbk43K3IvT2tuMDdmUUhFTUl6?=
 =?utf-8?B?NlZvZ2Z0d0pBVXlSOE8wVjdOcXBhL2YxTXlJRXVkNDhKYzRjOXlxZXMrdVY0?=
 =?utf-8?Q?IIkxU21UTKIBxyapdw5sGOgD0?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1c7908d0-7a59-467b-8346-08db15264a50
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2767.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Feb 2023 22:44:00.5131
 (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: CpNAsHmazlGAVjG8nBTq8NZNXCX6okarn+NvGkd5ZfcFTUN36+UaefYmFvRCJXXPnOzRnJjdAIr0Gwzl7KnpnA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB5311
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 2/22/2023 6:32 AM, 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/Documentation/virt/coco/sev-guest.rst b/Documentation/virt/coco/sev-guest.rst
>> index 11ea67c944df..6cad4226c348 100644
>> --- a/Documentation/virt/coco/sev-guest.rst
>> +++ b/Documentation/virt/coco/sev-guest.rst
>> @@ -145,6 +145,33 @@ The SNP_PLATFORM_STATUS command is used to query the SNP platform status. The
>>   status includes API major, minor version and more. See the SEV-SNP
>>   specification for further details.
>>   
>> +2.5 SNP_SET_EXT_CONFIG
>> +----------------------
>> +:Technology: sev-snp
>> +:Type: hypervisor ioctl cmd
>> +:Parameters (in): struct sev_data_snp_ext_config
>> +:Returns (out): 0 on success, -negative on error
>> +
>> +The SNP_SET_EXT_CONFIG is used to set the system-wide configuration such as
>> +reported TCB version in the attestation report. The command is similar to
>> +SNP_CONFIG command defined in the SEV-SNP spec. The main difference is the
>> +command also accepts an additional certificate blob defined in the GHCB
>> +specification.
>> +
>> +If the certs_address is zero, then the previous certificate blob will deleted.
>> +For more information on the certificate blob layout, see the GHCB spec
>> +(extended guest request message).
>> +
>> +2.6 SNP_GET_EXT_CONFIG
>> +----------------------
>> +:Technology: sev-snp
>> +:Type: hypervisor ioctl cmd
>> +:Parameters (in): struct sev_data_snp_ext_config
>> +:Returns (out): 0 on success, -negative on error
>> +
>> +The SNP_GET_EXT_CONFIG is used to query the system-wide configuration set
>> +through the SNP_SET_EXT_CONFIG.
>> +
>>   3. SEV-SNP CPUID Enforcement
>>   ============================
>>   
>> 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;
>> +			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 userspace provides an input buffer/length smaller then snp_certs_len, 
then the above returns the "required" certs length back to userspace.

And what is the issue if input.certs_len > sev->snp_certs_len, the 
buffer returned back to userspace is sev->snp_certs_len as below.

Thanks,
Ashish

>> +		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;
>> +}
>> +
>> +static int sev_ioctl_snp_set_config(struct sev_issue_cmd *argp, bool writable)
>> +{
>> +	struct sev_device *sev = psp_master->sev_data;
>> +	struct sev_user_data_ext_snp_config input;
>> +	struct sev_user_data_snp_config config;
>> +	void *certs = NULL;
>> +	int ret = 0;
>> +
>> +	if (!sev->snp_initialized || !argp->data)
>> +		return -EINVAL;
>> +
>> +	if (!writable)
>> +		return -EPERM;
>> +
>> +	memset(&input, 0, sizeof(input));
>> +
>> +	if (copy_from_user(&input, (void __user *)argp->data, sizeof(input)))
>> +		return -EFAULT;
>> +
>> +	/* Copy the certs from userspace */
>> +	if (input.certs_address) {
>> +		if (!input.certs_len || !IS_ALIGNED(input.certs_len, PAGE_SIZE))
>> +			return -EINVAL;
>> +
>> +		certs = psp_copy_user_blob(input.certs_address, input.certs_len);
>> +		if (IS_ERR(certs))
>> +			return PTR_ERR(certs);
>> +	}
>> +
>> +	/* Issue the PSP command to update the TCB version using the SNP_CONFIG. */
>> +	if (input.config_address) {
>> +		memset(&config, 0, sizeof(config));
>> +		if (copy_from_user(&config,
>> +				   (void __user *)input.config_address, sizeof(config))) {
>> +			ret = -EFAULT;
>> +			goto e_free;
>> +		}
>> +
>> +		ret = __sev_do_cmd_locked(SEV_CMD_SNP_CONFIG, &config, &argp->error);
>> +		if (ret)
>> +			goto e_free;
>> +
>> +		memcpy(&sev->snp_config, &config, sizeof(config));
>> +	}
>> +
>> +	/*
>> +	 * If the new certs are passed then cache it else free the old certs.
>> +	 */
>> +	mutex_lock(&sev->snp_certs_lock);
>> +	if (certs) {
>> +		kfree(sev->snp_certs_data);
>> +		sev->snp_certs_data = certs;
>> +		sev->snp_certs_len = input.certs_len;
>> +	} else {
>> +		kfree(sev->snp_certs_data);
>> +		sev->snp_certs_data = NULL;
>> +		sev->snp_certs_len = 0;
>> +	}
>> +	mutex_unlock(&sev->snp_certs_lock);
>> +
>> +	return 0;
>> +
>> +e_free:
>> +	kfree(certs);
>> +	return ret;
>> +}
>> +
>>   static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>>   {
>>   	void __user *argp = (void __user *)arg;
>> @@ -1847,6 +1963,12 @@ static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long arg)
>>   	case SNP_PLATFORM_STATUS:
>>   		ret = sev_ioctl_snp_platform_status(&input);
>>   		break;
>> +	case SNP_SET_EXT_CONFIG:
>> +		ret = sev_ioctl_snp_set_config(&input, writable);
>> +		break;
>> +	case SNP_GET_EXT_CONFIG:
>> +		ret = sev_ioctl_snp_get_config(&input);
>> +		break;
>>   	default:
>>   		ret = -EINVAL;
>>   		goto out;
>> @@ -1962,6 +2084,7 @@ int sev_dev_init(struct psp_device *psp)
>>   		goto e_sev;
>>   
>>   	sev->cmd_buf_backup = (uint8_t *)sev->cmd_buf + PAGE_SIZE;
>> +	mutex_init(&sev->snp_certs_lock);
>>   
>>   	psp->sev_data = sev;
>>   
>> diff --git a/drivers/crypto/ccp/sev-dev.h b/drivers/crypto/ccp/sev-dev.h
>> index 19d79f9d4212..41d5353d5bab 100644
>> --- a/drivers/crypto/ccp/sev-dev.h
>> +++ b/drivers/crypto/ccp/sev-dev.h
>> @@ -66,6 +66,10 @@ struct sev_device {
>>   
>>   	bool snp_initialized;
>>   	struct snp_host_map snp_host_map[MAX_SNP_HOST_MAP_BUFS];
>> +	void *snp_certs_data;
>> +	u32 snp_certs_len;
>> +	struct mutex snp_certs_lock;
>> +	struct sev_user_data_snp_config snp_config;
>>   };
>>   
>>   int sev_dev_init(struct psp_device *psp);
>> diff --git a/include/uapi/linux/psp-sev.h b/include/uapi/linux/psp-sev.h
>> index 5adfaea7df97..c20d37586d21 100644
>> --- a/include/uapi/linux/psp-sev.h
>> +++ b/include/uapi/linux/psp-sev.h
>> @@ -29,6 +29,8 @@ enum {
>>   	SEV_GET_ID,	/* This command is deprecated, use SEV_GET_ID2 */
>>   	SEV_GET_ID2,
>>   	SNP_PLATFORM_STATUS,
>> +	SNP_SET_EXT_CONFIG,
>> +	SNP_GET_EXT_CONFIG,
>>   
>>   	SEV_MAX,
>>   };
>> @@ -192,6 +194,21 @@ struct sev_user_data_snp_config {
>>   	__u8 rsvd1[52];
>>   } __packed;
>>   
>> +/**
>> + * struct sev_data_snp_ext_config - system wide configuration value for SNP.
>> + *
>> + * @config_address: address of the struct sev_user_data_snp_config or 0 when
>> + *		reported_tcb does not need to be updated.
>> + * @certs_address: address of extended guest request certificate chain or
>> + *              0 when previous certificate should be removed on SNP_SET_EXT_CONFIG.
>> + * @certs_len: length of the certs
>> + */
>> +struct sev_user_data_ext_snp_config {
>> +	__u64 config_address;		/* In */
>> +	__u64 certs_address;		/* In */
>> +	__u32 certs_len;		/* In */
>> +};
>> +
>>   /**
>>    * struct sev_issue_cmd - SEV ioctl parameters
>>    *
>