linux-kernel - Re: [PATCH v9 06/24] virt: sev-guest: Simplify VMPCK and sequence number assignments

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9b8c57e7-a871-6771-dcc0-99847bbbcbc0@amd.com>
Date: Wed, 19 Jun 2024 11:36:17 +0530
From: "Nikunj A. Dadhania" <nikunj@....com>
To: Tom Lendacky <thomas.lendacky@....com>, linux-kernel@...r.kernel.org,
 bp@...en8.de, x86@...nel.org, kvm@...r.kernel.org
Cc: mingo@...hat.com, tglx@...utronix.de, dave.hansen@...ux.intel.com,
 pgonda@...gle.com, seanjc@...gle.com, pbonzini@...hat.com
Subject: Re: [PATCH v9 06/24] virt: sev-guest: Simplify VMPCK and sequence
 number assignments



On 6/19/2024 2:57 AM, Tom Lendacky wrote:
> On 5/30/24 23:30, Nikunj A Dadhania wrote:
>> Preparatory patch to remove direct usage of VMPCK and message sequence
>> number in the SEV guest driver. Use arrays for the VM platform
>> communication key and message sequence number to simplify the function and
>> usage.
>>
>> Signed-off-by: Nikunj A Dadhania <nikunj@....com>
> 
> One minor comment below, otherwise, for the general logic of using an array:
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@....com>
> 
>> ---
>>  arch/x86/include/asm/sev.h              | 12 ++++-------
>>  drivers/virt/coco/sev-guest/sev-guest.c | 27 ++++---------------------
>>  2 files changed, 8 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index dbf17e66d52a..d06b08f7043c 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -118,6 +118,8 @@ struct sev_guest_platform_data {
>>  	u64 secrets_gpa;
>>  };
>>  
>> +#define VMPCK_MAX_NUM		4
>> +
>>  /*
>>   * The secrets page contains 96-bytes of reserved field that can be used by
>>   * the guest OS. The guest OS uses the area to save the message sequence
>> @@ -126,10 +128,7 @@ struct sev_guest_platform_data {
>>   * See the GHCB spec section Secret page layout for the format for this area.
>>   */
>>  struct secrets_os_area {
>> -	u32 msg_seqno_0;
>> -	u32 msg_seqno_1;
>> -	u32 msg_seqno_2;
>> -	u32 msg_seqno_3;
>> +	u32 msg_seqno[VMPCK_MAX_NUM];
>>  	u64 ap_jump_table_pa;
>>  	u8 rsvd[40];
>>  	u8 guest_usage[32];
>> @@ -145,10 +144,7 @@ struct snp_secrets_page {
>>  	u32 fms;
>>  	u32 rsvd2;
>>  	u8 gosvw[16];
>> -	u8 vmpck0[VMPCK_KEY_LEN];
>> -	u8 vmpck1[VMPCK_KEY_LEN];
>> -	u8 vmpck2[VMPCK_KEY_LEN];
>> -	u8 vmpck3[VMPCK_KEY_LEN];
>> +	u8 vmpck[VMPCK_MAX_NUM][VMPCK_KEY_LEN];
>>  	struct secrets_os_area os_area;
>>  	u8 rsvd3[3840];
>>  } __packed;
>> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
>> index 5c0cbdad9fa2..a3c0b22d2e14 100644
>> --- a/drivers/virt/coco/sev-guest/sev-guest.c
>> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
>> @@ -668,30 +668,11 @@ static const struct file_operations snp_guest_fops = {
>>  
>>  static u8 *get_vmpck(int id, struct snp_secrets_page *secrets, u32 **seqno)
>>  {
>> -	u8 *key = NULL;
>> -
>> -	switch (id) {
>> -	case 0:
>> -		*seqno = &secrets->os_area.msg_seqno_0;
>> -		key = secrets->vmpck0;
>> -		break;
>> -	case 1:
>> -		*seqno = &secrets->os_area.msg_seqno_1;
>> -		key = secrets->vmpck1;
>> -		break;
>> -	case 2:
>> -		*seqno = &secrets->os_area.msg_seqno_2;
>> -		key = secrets->vmpck2;
>> -		break;
>> -	case 3:
>> -		*seqno = &secrets->os_area.msg_seqno_3;
>> -		key = secrets->vmpck3;
>> -		break;
>> -	default:
>> -		break;
>> -	}
>> +	if ((id + 1) > VMPCK_MAX_NUM)
>> +		return NULL;
> 
> This looks a bit confusing to me, because of the way it has to be
> written with the "+ 1". I wonder if something like the following would
> read better:
> 
> 	switch (id) {
> 	case 0 ... 3:
> 		*seqno = &secrets->os_area.msg_seqno[id];
> 		return secrets->vmpck[id];
> 	default:
> 		return NULL;
> 	}
>
> Just my opinion, if others are fine with it, then that's fine.

I have separated patch 6 and 7 for better code review and modular changes.

The next patch simplifes this further to:

static inline u8 *get_vmpck(struct snp_guest_dev *snp_dev)
{
	return snp_dev->secrets->vmpck[snp_dev->vmpck_id];
}

static bool assign_vmpck(struct snp_guest_dev *dev, unsigned int vmpck_id)
{
	if ((vmpck_id + 1) > VMPCK_MAX_NUM)
		return false;

	dev->vmpck_id = vmpck_id;

	return true;
}


Regards
Nikunj