linux-kernel - Re: [PATCH v10 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: <20240625170450.GMZnr4surYmBPd94lC@fat_crate.local>
Date: Tue, 25 Jun 2024 19:04:50 +0200
From: Borislav Petkov <bp@...en8.de>
To: Nikunj A Dadhania <nikunj@....com>
Cc: linux-kernel@...r.kernel.org, thomas.lendacky@....com, x86@...nel.org,
	kvm@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
	dave.hansen@...ux.intel.com, pgonda@...gle.com, seanjc@...gle.com,
	pbonzini@...hat.com
Subject: Re: [PATCH v10 06/24] virt: sev-guest: Simplify VMPCK and sequence
 number assignments

On Fri, Jun 21, 2024 at 06:08:45PM +0530, Nikunj A Dadhania wrote:
> Preparatory patch to remove direct usage of VMPCK and message sequence

"Prepare the code for removing... "

>From Documentation/process/submitting-patches.rst:

"Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behaviour."

> number in the SEV guest driver.

remove, because...?

> 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>
> 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 2ac899adcbf6..473760208764 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];
> @@ -214,10 +213,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 vmsa_tweak_bitmap[64];
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 61e190ecfa3a..a5602c84769f 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -678,30 +678,11 @@ static const struct file_operations snp_guest_fops = {
>  
>  static u8 *get_vmpck(int id, struct snp_secrets_page *secrets, u32 **seqno)

Why is this a separate function when it is used only once?

>  {
> -	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 < VMPCK_MAX_NUM))
> +		return NULL;

Or

	if (id >= VMPCK_MAX_NUM)
		return NULL;

?

Also that id needs to be unsigned as it is an array index.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette