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: <64164798-3055-c745-0bc1-bbecc1dd0421@amd.com>
Date: Tue, 18 Jun 2024 16:27:38 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: Nikunj A Dadhania <nikunj@....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 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.

Thanks,
Tom

>  
> -	return key;
> +	*seqno = &secrets->os_area.msg_seqno[id];
> +	return secrets->vmpck[id];
>  }
>  
>  struct snp_msg_report_resp_hdr {