linux-kernel - Re: [PATCH v3 09/13] KVM: x86: emulator/smm: use smram struct for 32 bit smram load/restore

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: <YwamHYLokbGX96cG@google.com>
Date:   Wed, 24 Aug 2022 22:28:45 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     kvm@...r.kernel.org, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        linux-kernel@...r.kernel.org, Wanpeng Li <wanpengli@...cent.com>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        Jim Mattson <jmattson@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>, Joerg Roedel <joro@...tes.org>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v3 09/13] KVM: x86: emulator/smm: use smram struct for 32
 bit smram load/restore

On Wed, Aug 03, 2022, Maxim Levitsky wrote:
> Use kvm_smram_state_32 struct to save/restore 32 bit SMM state
> (used when X86_FEATURE_LM is not present in the guest CPUID).
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@...hat.com>
> ---
>  arch/x86/kvm/emulate.c | 81 +++++++++++++++---------------------------
>  arch/x86/kvm/x86.c     | 75 +++++++++++++++++---------------------
>  2 files changed, 60 insertions(+), 96 deletions(-)
> 
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 610978d00b52b0..3339d542a25439 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -2371,25 +2371,17 @@ static void rsm_set_desc_flags(struct desc_struct *desc, u32 flags)
>  	desc->type = (flags >>  8) & 15;
>  }
>  
> -static int rsm_load_seg_32(struct x86_emulate_ctxt *ctxt, const char *smstate,
> +static void rsm_load_seg_32(struct x86_emulate_ctxt *ctxt,
> +			   const struct kvm_smm_seg_state_32 *state,

Alignment is off by one.

> +			   u16 selector,
>  			   int n)

These can go on a single line.

static void rsm_load_seg_32(struct x86_emulate_ctxt *ctxt,
			    const struct kvm_smm_seg_state_32 *state,
			    u16 selector, int n)

>  	struct desc_struct desc;
> -	int offset;
> -	u16 selector;
> -
> -	selector = GET_SMSTATE(u32, smstate, 0x7fa8 + n * 4);
> -
> -	if (n < 3)
> -		offset = 0x7f84 + n * 12;
> -	else
> -		offset = 0x7f2c + (n - 3) * 12;
>  
> -	set_desc_base(&desc,      GET_SMSTATE(u32, smstate, offset + 8));
> -	set_desc_limit(&desc,     GET_SMSTATE(u32, smstate, offset + 4));
> -	rsm_set_desc_flags(&desc, GET_SMSTATE(u32, smstate, offset));
> +	set_desc_base(&desc,      state->base);
> +	set_desc_limit(&desc,     state->limit);
> +	rsm_set_desc_flags(&desc, state->flags);
>  	ctxt->ops->set_segment(ctxt, selector, &desc, 0, n);
> -	return X86EMUL_CONTINUE;
>  }
>  

...

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index cbbe49bdc58787..6abe35f7687e2c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9833,22 +9833,18 @@ static u32 enter_smm_get_segment_flags(struct kvm_segment *seg)
>  	return flags;
>  }
>  
> -static void enter_smm_save_seg_32(struct kvm_vcpu *vcpu, char *buf, int n)
> +static void enter_smm_save_seg_32(struct kvm_vcpu *vcpu,
> +					 struct kvm_smm_seg_state_32 *state,
> +					 u32 *selector,
> +					 int n)

Similar issues here.

static void enter_smm_save_seg_32(struct kvm_vcpu *vcpu,
				  struct kvm_smm_seg_state_32 *state,
				  u32 *selector, int n)
>  {
>  	struct kvm_segment seg;
> -	int offset;
>  
>  	kvm_get_segment(vcpu, &seg, n);
> -	put_smstate(u32, buf, 0x7fa8 + n * 4, seg.selector);
> -
> -	if (n < 3)
> -		offset = 0x7f84 + n * 12;
> -	else
> -		offset = 0x7f2c + (n - 3) * 12;
> -
> -	put_smstate(u32, buf, offset + 8, seg.base);
> -	put_smstate(u32, buf, offset + 4, seg.limit);
> -	put_smstate(u32, buf, offset, enter_smm_get_segment_flags(&seg));
> +	*selector = seg.selector;
> +	state->base = seg.base;
> +	state->limit = seg.limit;
> +	state->flags = enter_smm_get_segment_flags(&seg);
>  }
>  
>  #ifdef CONFIG_X86_64
> @@ -9869,54 +9865,47 @@ static void enter_smm_save_seg_64(struct kvm_vcpu *vcpu, char *buf, int n)
>  }
>  #endif
>  
> -static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, char *buf)
> +static void enter_smm_save_state_32(struct kvm_vcpu *vcpu, struct kvm_smram_state_32 *smram)

Please wrap, no reason to run long.

static void enter_smm_save_state_32(struct kvm_vcpu *vcpu,
				    struct kvm_smram_state_32 *smram)