[PATCH] arm64/kvm: Prohibit guest LOR accesses

[Mark Rutland]

On Tue, Feb 13, 2018 at 11:27:42AM +0100, Christoffer Dall wrote:
> Hi Mark,

Hi Christoffer,

> On Mon, Feb 12, 2018 at 11:14:24AM +0000, Mark Rutland wrote:
> > We don't currently limit guest accesses to the LOR registers, which we
> > neither virtualize nor context-switch. As such, guests are provided with
> > unusable information/controls, and are not isolated from each other (or
> > the host).
> > 
> > To prevent these issues, we can trap register accesses and present the
> > illusion that no LORegions are implemented.
> 
> Shouldn't we then also hide this from the ID register similar to what we
> do for SVE at the moment, using something like this (untested):
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index a828a209716f..b7ee414aee67 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1064,6 +1064,12 @@ static u64 read_id_reg(struct sys_reg_desc const *r, bool raz)
>  				    task_pid_nr(current));
>  
>  		val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
> +	} else if (id == SYS_ID_AA64MMFR1_EL) {
> +		if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
> +			pr_err_once("kvm [%i]: LORegions unsupported for guests, suppressing\n",
> +				    task_pid_nr(current));
> +
> +		val &= ~(0xfUL << ID_AA64MMFR1_LOR_SHIFT);
>  	}
>  
>  	return val;

Sure. With that, it would make sense for the trapped LOR* register
accesses to UNDEF, as they logically shouldn't exist.

[...]

> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index 50a43c7b97ca..67b10b7a07bf 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -175,6 +175,17 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
> >  		return read_zero(vcpu, p);
> >  }
> >  
> > +static bool trap_raz_ro(struct kvm_vcpu *vcpu,
> > +			struct sys_reg_params *p,
> > +			const struct sys_reg_desc *r)
> > +{
> > +	if (!p->is_write)
> > +		return read_zero(vcpu, p);
> > +
> > +	kvm_inject_undefined(vcpu);
> > +	return false;
> > +}
> > +
> 
> I'm a little unclear if this is stricly the right thing to do.  I can't
> seem to find anything specific about writes to this register (which has
> some text about RW fields but only specifies it can be *read* with an
> MRS instruction), so it seems to me that this should either be
> trap_raz_wi(), or call write_to_read_only() if we expect a write to
> undef at EL1 rather than trap to EL2.  On the other hand, if the
> architecture is unclear, then we're at least enforcing one
> interpretation here I suppose.

Having looked at the ARM ARM, I think write_to_read_only() is the right
thing to do.

It's not clear to me whether HCR_EL2.TLOR traps writes to LORID_EL1, or
leaves these UNDEFINED at EL1. In the description of traps and enables
it's stated that HCR_EL2.TLOR traps "Non-secure accesses to this
register from EL1", which could be read to mean either. I'll see if I
can get to the bottom of that.

I don't think trap_raz_wi(), as LORID_EL1 is never RW at EL1, and a
write wound UNDEF were HCR_EL2.TLOR clear.

Thanks,
Mark.