linux-kernel - Re: [PATCH 4/5] KVM: x86: Wrong register masking in 64-bit mode

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]

Date:	Wed, 07 May 2014 10:50:15 -0400
From:	Bandan Das <bsd@...hat.com>
To:	Nadav Amit <namit@...technion.ac.il>
Cc:	mtosatti@...hat.com, pbonzini@...hat.com, hpa@...or.com,
	gleb@...nel.org, tglx@...utronix.de, mingo@...hat.com,
	x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] KVM: x86: Wrong register masking in 64-bit mode

Nadav Amit <namit@...technion.ac.il> writes:

> 32-bit operations are zero extended in 64-bit mode. Currently, the code does
> not handle them correctly and keeps the high bits. In 16-bit mode, the high
> 32-bits are kept intact.
>
> In addition, although it is not well-documented, when address override prefix

It would be helpful to have a pointer in the SDM especially for cases 
that are not well-documented.

> is used with REP-string instruction, RCX high half is zeroed even if ECX was
> zero on the first iteration (as if an assignment was performed to ECX).
>
> Signed-off-by: Nadav Amit <namit@...technion.ac.il>
> ---
>  arch/x86/kvm/emulate.c | 38 +++++++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
> index 743e8e3..6833b41 100644
> --- a/arch/x86/kvm/emulate.c
> +++ b/arch/x86/kvm/emulate.c
> @@ -434,9 +434,21 @@ static int emulator_check_intercept(struct x86_emulate_ctxt *ctxt,
>  	return ctxt->ops->intercept(ctxt, &info, stage);
>  }
>  
> -static void assign_masked(ulong *dest, ulong src, ulong mask)
> +static void assign_masked(ulong *dest, ulong src, int bytes)
>  {
> -	*dest = (*dest & ~mask) | (src & mask);
> +	switch (bytes) {
> +	case 2:
> +		*dest = (u16)src | (*dest & ~0xfffful);
> +		break;
> +	case 4:
> +		*dest = (u32)src;
> +		break;
> +	case 8:
> +		*dest = src;
> +		break;
> +	default:
> +		BUG();

IIRC, Paolo mentioned that a WARN() is preferable. But I see
a lot other places where BUG() is called, maybe, he can confirm.

> +	}
>  }
>  
>  static inline unsigned long ad_mask(struct x86_emulate_ctxt *ctxt)
> @@ -476,26 +488,20 @@ register_address(struct x86_emulate_ctxt *ctxt, unsigned long reg)
>  	return address_mask(ctxt, reg);
>  }
>  
> -static void masked_increment(ulong *reg, ulong mask, int inc)
> +static void masked_increment(ulong *reg, int size, int inc)
>  {
> -	assign_masked(reg, *reg + inc, mask);
> +	assign_masked(reg, *reg + inc, size);
>  }
>  
>  static inline void
>  register_address_increment(struct x86_emulate_ctxt *ctxt, unsigned long *reg, int inc)
>  {
> -	ulong mask;
> -
> -	if (ctxt->ad_bytes == sizeof(unsigned long))
> -		mask = ~0UL;
> -	else
> -		mask = ad_mask(ctxt);
> -	masked_increment(reg, mask, inc);
> +	masked_increment(reg, ctxt->ad_bytes, inc);
>  }
>  
>  static void rsp_increment(struct x86_emulate_ctxt *ctxt, int inc)
>  {
> -	masked_increment(reg_rmw(ctxt, VCPU_REGS_RSP), stack_mask(ctxt), inc);
> +	masked_increment(reg_rmw(ctxt, VCPU_REGS_RSP), stack_size(ctxt), inc);
>  }
>  
>  static inline void jmp_rel(struct x86_emulate_ctxt *ctxt, int rel)
> @@ -1712,17 +1718,17 @@ static int em_enter(struct x86_emulate_ctxt *ctxt)
>  	if (rc != X86EMUL_CONTINUE)
>  		return rc;
>  	assign_masked(reg_rmw(ctxt, VCPU_REGS_RBP), reg_read(ctxt, VCPU_REGS_RSP),
> -		      stack_mask(ctxt));
> +		      stack_size(ctxt));
>  	assign_masked(reg_rmw(ctxt, VCPU_REGS_RSP),
>  		      reg_read(ctxt, VCPU_REGS_RSP) - frame_size,
> -		      stack_mask(ctxt));
> +		      stack_size(ctxt));
>  	return X86EMUL_CONTINUE;
>  }
>  
>  static int em_leave(struct x86_emulate_ctxt *ctxt)
>  {
>  	assign_masked(reg_rmw(ctxt, VCPU_REGS_RSP), reg_read(ctxt, VCPU_REGS_RBP),
> -		      stack_mask(ctxt));
> +		      stack_size(ctxt));
>  	return emulate_pop(ctxt, reg_rmw(ctxt, VCPU_REGS_RBP), ctxt->op_bytes);
>  }
>  
> @@ -4570,6 +4576,8 @@ int x86_emulate_insn(struct x86_emulate_ctxt *ctxt)
>  	if (ctxt->rep_prefix && (ctxt->d & String)) {
>  		/* All REP prefixes have the same first termination condition */
>  		if (address_mask(ctxt, reg_read(ctxt, VCPU_REGS_RCX)) == 0) {
> +			assign_masked(reg_write(ctxt, VCPU_REGS_RCX), 0,
> +				      ctxt->ad_bytes);
>  			ctxt->eip = ctxt->_eip;
>  			goto done;
>  		}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/