[PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace

[PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace

[Andrew Jones]

ID registers are RAZ until they've been allocated a purpose, but
that doesn't mean they should be removed from the KVM_GET_REG_LIST
list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
is hidden from userspace when its function is not present. Removing
the userspace visibility checks is enough to reexpose it, as it
already behaves as RAZ when the function is not present.

Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
Cc: <stable@vger.kernel.org> # v5.2+
Reported-by: 张东旭 <xu910121@sina.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/arm64/kvm/sys_regs.c | 18 +-----------------
 1 file changed, 1 insertion(+), 17 deletions(-)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index fb12d3ef423a..6ff0c15531ca 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
 	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
 }
 
-/* Visibility overrides for SVE-specific ID registers */
-static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
-				      const struct sys_reg_desc *rd)
-{
-	if (vcpu_has_sve(vcpu))
-		return 0;
-
-	return REG_HIDDEN_USER;
-}
-
 /* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
 static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
 {
@@ -1231,9 +1221,6 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 {
 	u64 val;
 
-	if (WARN_ON(!vcpu_has_sve(vcpu)))
-		return -ENOENT;
-
 	val = guest_id_aa64zfr0_el1(vcpu);
 	return reg_to_user(uaddr, &val, reg->id);
 }
@@ -1246,9 +1233,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
 	int err;
 	u64 val;
 
-	if (WARN_ON(!vcpu_has_sve(vcpu)))
-		return -ENOENT;
-
 	err = reg_from_user(&val, uaddr, id);
 	if (err)
 		return err;
@@ -1518,7 +1502,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	ID_SANITISED(ID_AA64PFR1_EL1),
 	ID_UNALLOCATED(4,2),
 	ID_UNALLOCATED(4,3),
-	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
+	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
 	ID_UNALLOCATED(4,5),
 	ID_UNALLOCATED(4,6),
 	ID_UNALLOCATED(4,7),
-- 
2.26.2

Re: [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace

[Dave Martin]

On Mon, Nov 02, 2020 at 07:50:35PM +0100, Andrew Jones wrote:
> ID registers are RAZ until they've been allocated a purpose, but
> that doesn't mean they should be removed from the KVM_GET_REG_LIST
> list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
> is hidden from userspace when its function is not present. Removing
> the userspace visibility checks is enough to reexpose it, as it
> already behaves as RAZ when the function is not present.

Pleae state what the patch does.  (The subject line serves as a summary
of that, but the commit message should make sense without it.)

Also, how exactly !vcpu_has_sve() causes ID_AA64ZFR0_EL1 to behave as
RAZ with this change?  (I'm not saying it doesn't, but the code is not
trivial to follow...)

> 
> Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
> Cc: <stable@vger.kernel.org> # v5.2+
> Reported-by: 张东旭 <xu910121@sina.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 18 +-----------------
>  1 file changed, 1 insertion(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index fb12d3ef423a..6ff0c15531ca 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
>  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
>  }
>  
> -/* Visibility overrides for SVE-specific ID registers */
> -static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
> -				      const struct sys_reg_desc *rd)
> -{
> -	if (vcpu_has_sve(vcpu))
> -		return 0;
> -
> -	return REG_HIDDEN_USER;

In light of this change, I think that REG_HIDDEN_GUEST and
REG_HIDDEN_USER are always either both set or both clear.  Given the
discussion on this issue, I'm thinking it probably doesn't even make
sense for these to be independent (?)

If REG_HIDDEN_USER is really redundant, I suggest stripping it out and
simplifying the code appropriately.

(In effect, I think your RAZ flag will do correctly what REG_HIDDEN_USER
was trying to achieve.)

> -}
> -
>  /* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
>  static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
>  {
> @@ -1231,9 +1221,6 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>  {
>  	u64 val;
>  
> -	if (WARN_ON(!vcpu_has_sve(vcpu)))
> -		return -ENOENT;
> -
>  	val = guest_id_aa64zfr0_el1(vcpu);
>  	return reg_to_user(uaddr, &val, reg->id);
>  }
> @@ -1246,9 +1233,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
>  	int err;
>  	u64 val;
>  
> -	if (WARN_ON(!vcpu_has_sve(vcpu)))
> -		return -ENOENT;
> -
>  	err = reg_from_user(&val, uaddr, id);
>  	if (err)
>  		return err;
> @@ -1518,7 +1502,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	ID_SANITISED(ID_AA64PFR1_EL1),
>  	ID_UNALLOCATED(4,2),
>  	ID_UNALLOCATED(4,3),
> -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
> +	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
>  	ID_UNALLOCATED(4,5),
>  	ID_UNALLOCATED(4,6),
>  	ID_UNALLOCATED(4,7),

Otherwise looks reasonable.

Cheers
---Dave

Re: [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace

[Andrew Jones]

On Tue, Nov 03, 2020 at 11:18:19AM +0000, Dave Martin wrote:
> On Mon, Nov 02, 2020 at 07:50:35PM +0100, Andrew Jones wrote:
> > ID registers are RAZ until they've been allocated a purpose, but
> > that doesn't mean they should be removed from the KVM_GET_REG_LIST
> > list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
> > is hidden from userspace when its function is not present. Removing
> > the userspace visibility checks is enough to reexpose it, as it
> > already behaves as RAZ when the function is not present.
> 
> Pleae state what the patch does.  (The subject line serves as a summary
> of that, but the commit message should make sense without it.)

I don't like "This patch ..." type of sentences in the commit message,
but unless you have a better suggestion, then I'd just add a sentence
like

"This patch ensures we still expose sysreg '3, 0, 0, 4, 4'
(ID_AA64ZFR0_EL1) to userspace as RAZ when SVE is not implemented."

> 
> Also, how exactly !vcpu_has_sve() causes ID_AA64ZFR0_EL1 to behave as
> RAZ with this change?  (I'm not saying it doesn't, but the code is not
> trivial to follow...)

guest_id_aa64zfr0_el1() returns zero for the register when !vcpu_has_sve(),
and all the accessors (userspace and guest) build on it.

I'm not sure how helpful it would be to add that sentence to the commit
message though.

> 
> > 
> > Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
> > Cc: <stable@vger.kernel.org> # v5.2+
> > Reported-by: 张东旭 <xu910121@sina.com>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arch/arm64/kvm/sys_regs.c | 18 +-----------------
> >  1 file changed, 1 insertion(+), 17 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index fb12d3ef423a..6ff0c15531ca 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> >  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
> >  }
> >  
> > -/* Visibility overrides for SVE-specific ID registers */
> > -static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
> > -				      const struct sys_reg_desc *rd)
> > -{
> > -	if (vcpu_has_sve(vcpu))
> > -		return 0;
> > -
> > -	return REG_HIDDEN_USER;
> 
> In light of this change, I think that REG_HIDDEN_GUEST and
> REG_HIDDEN_USER are always either both set or both clear.  Given the
> discussion on this issue, I'm thinking it probably doesn't even make
> sense for these to be independent (?)
> 
> If REG_HIDDEN_USER is really redundant, I suggest stripping it out and
> simplifying the code appropriately.
> 
> (In effect, I think your RAZ flag will do correctly what REG_HIDDEN_USER
> was trying to achieve.)

We could consolidate REG_HIDDEN_GUEST and REG_HIDDEN_USER into REG_HIDDEN,
which ZCR_EL1 and ptrauth registers will still use.

> 
> > -}
> > -
> >  /* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
> >  static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
> >  {
> > @@ -1231,9 +1221,6 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> >  {
> >  	u64 val;
> >  
> > -	if (WARN_ON(!vcpu_has_sve(vcpu)))
> > -		return -ENOENT;
> > -
> >  	val = guest_id_aa64zfr0_el1(vcpu);
> >  	return reg_to_user(uaddr, &val, reg->id);
> >  }
> > @@ -1246,9 +1233,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> >  	int err;
> >  	u64 val;
> >  
> > -	if (WARN_ON(!vcpu_has_sve(vcpu)))
> > -		return -ENOENT;
> > -
> >  	err = reg_from_user(&val, uaddr, id);
> >  	if (err)
> >  		return err;
> > @@ -1518,7 +1502,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> >  	ID_SANITISED(ID_AA64PFR1_EL1),
> >  	ID_UNALLOCATED(4,2),
> >  	ID_UNALLOCATED(4,3),
> > -	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
> > +	{ SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
> >  	ID_UNALLOCATED(4,5),
> >  	ID_UNALLOCATED(4,6),
> >  	ID_UNALLOCATED(4,7),
> 
> Otherwise looks reasonable.
>

Thanks,
drew

Re: [PATCH v2 1/3] KVM: arm64: Don't hide ID registers from userspace

[Dave Martin]

On Tue, Nov 03, 2020 at 02:32:15PM +0100, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 11:18:19AM +0000, Dave Martin wrote:
> > On Mon, Nov 02, 2020 at 07:50:35PM +0100, Andrew Jones wrote:
> > > ID registers are RAZ until they've been allocated a purpose, but
> > > that doesn't mean they should be removed from the KVM_GET_REG_LIST
> > > list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
> > > is hidden from userspace when its function is not present. Removing
> > > the userspace visibility checks is enough to reexpose it, as it
> > > already behaves as RAZ when the function is not present.
> > 
> > Pleae state what the patch does.  (The subject line serves as a summary
> > of that, but the commit message should make sense without it.)
> 
> I don't like "This patch ..." type of sentences in the commit message,
> but unless you have a better suggestion, then I'd just add a sentence
> like
> 
> "This patch ensures we still expose sysreg '3, 0, 0, 4, 4'
> (ID_AA64ZFR0_EL1) to userspace as RAZ when SVE is not implemented."

I'm not sure the sysreg encoding number is really needed.
submitting-patches.rst also says such statements should be in the
imperative.  Why not delete the "Removing the userspace visibility
checks..." sentence above and writing:

	Expose ID_AA64ZFR0_EL1 to userspace as RAZ when SVE is not
	implemented.

	Removing the userspace visibility checks is enough to reexpose it,
	as it already behaves as RAZ for the guest when SVE is not present.

(The background to this gripe is that "traditional" mailers may invoke a
standalone editor on the message body when composing reply, so the
subject line may not be visible...)

> 
> > 
> > Also, how exactly !vcpu_has_sve() causes ID_AA64ZFR0_EL1 to behave as
> > RAZ with this change?  (I'm not saying it doesn't, but the code is not
> > trivial to follow...)
> 
> guest_id_aa64zfr0_el1() returns zero for the register when !vcpu_has_sve(),
> and all the accessors (userspace and guest) build on it.
> 
> I'm not sure how helpful it would be to add that sentence to the commit
> message though.

No worries, I don't think you need to add anthing.  I figured out how
this works after my previously reply, so you can put my question down to
me being slow on the uptake...

> 
> > 
> > > 
> > > Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
> > > Cc: <stable@vger.kernel.org> # v5.2+
> > > Reported-by: 张东旭 <xu910121@sina.com>
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > >  arch/arm64/kvm/sys_regs.c | 18 +-----------------
> > >  1 file changed, 1 insertion(+), 17 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index fb12d3ef423a..6ff0c15531ca 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> > >  	return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
> > >  }
> > >  
> > > -/* Visibility overrides for SVE-specific ID registers */
> > > -static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
> > > -				      const struct sys_reg_desc *rd)
> > > -{
> > > -	if (vcpu_has_sve(vcpu))
> > > -		return 0;
> > > -
> > > -	return REG_HIDDEN_USER;
> > 
> > In light of this change, I think that REG_HIDDEN_GUEST and
> > REG_HIDDEN_USER are always either both set or both clear.  Given the
> > discussion on this issue, I'm thinking it probably doesn't even make
> > sense for these to be independent (?)
> > 
> > If REG_HIDDEN_USER is really redundant, I suggest stripping it out and
> > simplifying the code appropriately.
> > 
> > (In effect, I think your RAZ flag will do correctly what REG_HIDDEN_USER
> > was trying to achieve.)
> 
> We could consolidate REG_HIDDEN_GUEST and REG_HIDDEN_USER into REG_HIDDEN,
> which ZCR_EL1 and ptrauth registers will still use.

Sounds good to me.  Getting rid of _both_ the old names well help avoid
accidents too.

[...]

Cheers
---Dave