[v5,5/5] arm64/kvm: control accessibility of ptrauth key registers
diff mbox series

Message ID 1548658727-14271-6-git-send-email-amit.kachhap@arm.com
State New
Headers show
Series
  • Add ARMv8.3 pointer authentication for kvm guest
Related show

Commit Message

Amit Daniel Kachhap Jan. 28, 2019, 6:58 a.m. UTC
According to userspace settings, ptrauth key registers are conditionally
present in guest system register list based on user specified flag
KVM_ARM_VCPU_PTRAUTH.

Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Christoffer Dall <christoffer.dall@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: kvmarm@lists.cs.columbia.edu
Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 Documentation/arm64/pointer-authentication.txt |  3 ++
 arch/arm64/kvm/sys_regs.c                      | 42 +++++++++++++++++++-------
 2 files changed, 34 insertions(+), 11 deletions(-)

Comments

Kristina Martsenko Feb. 13, 2019, 5:35 p.m. UTC | #1
On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> According to userspace settings, ptrauth key registers are conditionally
> present in guest system register list based on user specified flag
> KVM_ARM_VCPU_PTRAUTH.
> 
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Christoffer Dall <christoffer.dall@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> Cc: kvmarm@lists.cs.columbia.edu
> Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  Documentation/arm64/pointer-authentication.txt |  3 ++
>  arch/arm64/kvm/sys_regs.c                      | 42 +++++++++++++++++++-------
>  2 files changed, 34 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
> index 0529a7d..3be4ee1 100644
> --- a/Documentation/arm64/pointer-authentication.txt
> +++ b/Documentation/arm64/pointer-authentication.txt
> @@ -87,3 +87,6 @@ created by passing a flag (KVM_ARM_VCPU_PTRAUTH) requesting this feature
>  to be enabled. Without this flag, pointer authentication is not enabled
>  in KVM guests and attempted use of the feature will result in an UNDEFINED
>  exception being injected into the guest.
> +
> +Additionally, when KVM_ARM_VCPU_PTRAUTH is not set then KVM will filter
> +out the authentication key registers from userspace.
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 2546a65..b46a78e 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1334,12 +1334,6 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
>  	{ SYS_DESC(SYS_TCR_EL1), access_vm_reg, reset_val, TCR_EL1, 0 },
>  
> -	PTRAUTH_KEY(APIA),
> -	PTRAUTH_KEY(APIB),
> -	PTRAUTH_KEY(APDA),
> -	PTRAUTH_KEY(APDB),
> -	PTRAUTH_KEY(APGA),
> -
>  	{ SYS_DESC(SYS_AFSR0_EL1), access_vm_reg, reset_unknown, AFSR0_EL1 },
>  	{ SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
>  	{ SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
> @@ -1491,6 +1485,14 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_FPEXC32_EL2), NULL, reset_val, FPEXC32_EL2, 0x70 },
>  };
>  
> +static const struct sys_reg_desc ptrauth_reg_descs[] = {
> +	PTRAUTH_KEY(APIA),
> +	PTRAUTH_KEY(APIB),
> +	PTRAUTH_KEY(APDA),
> +	PTRAUTH_KEY(APDB),
> +	PTRAUTH_KEY(APGA),
> +};
> +
>  static bool trap_dbgidr(struct kvm_vcpu *vcpu,
>  			struct sys_reg_params *p,
>  			const struct sys_reg_desc *r)
> @@ -2093,6 +2095,8 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
>  	r = find_reg(params, table, num);
>  	if (!r)
>  		r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> +	if (!r && kvm_arm_vcpu_ptrauth_allowed(vcpu))
> +		r = find_reg(params, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>  
>  	if (likely(r)) {
>  		perform_access(vcpu, params, r);
> @@ -2206,6 +2210,8 @@ static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
>  	r = find_reg_by_id(id, &params, table, num);
>  	if (!r)
>  		r = find_reg(&params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> +	if (!r && kvm_arm_vcpu_ptrauth_allowed(vcpu))
> +		r = find_reg(&params, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>  
>  	/* Not saved in the sys_reg array and not otherwise accessible? */
>  	if (r && !(r->reg || r->get_user))
> @@ -2487,18 +2493,22 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd,
>  }
>  
>  /* Assumed ordered tables, see kvm_sys_reg_table_init. */
> -static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
> +static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
> +			const struct sys_reg_desc *desc, unsigned int len)
>  {
>  	const struct sys_reg_desc *i1, *i2, *end1, *end2;
>  	unsigned int total = 0;
>  	size_t num;
>  	int err;
>  
> +	if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
> +		return total;
> +
>  	/* We check for duplicates here, to allow arch-specific overrides. */
>  	i1 = get_target_table(vcpu->arch.target, true, &num);
>  	end1 = i1 + num;
> -	i2 = sys_reg_descs;
> -	end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
> +	i2 = desc;
> +	end2 = desc + len;
>  
>  	BUG_ON(i1 == end1 || i2 == end2);
>  
> @@ -2526,7 +2536,10 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
>  {
>  	return ARRAY_SIZE(invariant_sys_regs)
>  		+ num_demux_regs()
> -		+ walk_sys_regs(vcpu, (u64 __user *)NULL);
> +		+ walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
> +				ARRAY_SIZE(sys_reg_descs))
> +		+ walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
> +				ARRAY_SIZE(ptrauth_reg_descs));
>  }
>  
>  int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> @@ -2541,7 +2554,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>  		uindices++;
>  	}
>  
> -	err = walk_sys_regs(vcpu, uindices);
> +	err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> +	if (err < 0)
> +		return err;
> +	uindices += err;
> +
> +	err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>  	if (err < 0)
>  		return err;
>  	uindices += err;
> @@ -2575,6 +2593,7 @@ void kvm_sys_reg_table_init(void)
>  	BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
>  	BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
>  	BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
> +	BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
>  
>  	/* We abuse the reset function to overwrite the table itself. */
>  	for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
> @@ -2616,6 +2635,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>  
>  	/* Generic chip reset first (so target could override). */
>  	reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> +	reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>  
>  	table = get_target_table(vcpu->arch.target, true, &num);
>  	reset_sys_reg_descs(vcpu, table, num);

This isn't very scalable, since we'd need to duplicate all the above
code every time we add new system registers that are conditionally
accessible.

It seems that the SVE patches [1] solved this problem by adding a
check_present() callback into struct sys_reg_desc. It probably makes
sense to rebase onto that patch and just implement the callback for the
ptrauth key registers as well.

[1] https://lore.kernel.org/linux-arm-kernel/1547757219-19439-13-git-send-email-Dave.Martin@arm.com/

Thanks,
Kristina
Dave Martin Feb. 13, 2019, 5:54 p.m. UTC | #2
On Wed, Feb 13, 2019 at 05:35:46PM +0000, Kristina Martsenko wrote:
> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
> > According to userspace settings, ptrauth key registers are conditionally
> > present in guest system register list based on user specified flag
> > KVM_ARM_VCPU_PTRAUTH.
> >
> > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Christoffer Dall <christoffer.dall@arm.com>
> > Cc: Marc Zyngier <marc.zyngier@arm.com>
> > Cc: Kristina Martsenko <kristina.martsenko@arm.com>
> > Cc: kvmarm@lists.cs.columbia.edu
> > Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
> > Cc: Will Deacon <will.deacon@arm.com>
> > ---
> >  Documentation/arm64/pointer-authentication.txt |  3 ++
> >  arch/arm64/kvm/sys_regs.c                      | 42 +++++++++++++++++++-------
> >  2 files changed, 34 insertions(+), 11 deletions(-)
> >

[...]

> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c

[...]

> > @@ -2487,18 +2493,22 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd,
> >  }
> >
> >  /* Assumed ordered tables, see kvm_sys_reg_table_init. */
> > -static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
> > +static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
> > +const struct sys_reg_desc *desc, unsigned int len)
> >  {
> >  const struct sys_reg_desc *i1, *i2, *end1, *end2;
> >  unsigned int total = 0;
> >  size_t num;
> >  int err;
> >
> > +if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
> > +return total;
> > +
> >  /* We check for duplicates here, to allow arch-specific overrides. */
> >  i1 = get_target_table(vcpu->arch.target, true, &num);
> >  end1 = i1 + num;
> > -i2 = sys_reg_descs;
> > -end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
> > +i2 = desc;
> > +end2 = desc + len;
> >
> >  BUG_ON(i1 == end1 || i2 == end2);
> >
> > @@ -2526,7 +2536,10 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
> >  {
> >  return ARRAY_SIZE(invariant_sys_regs)
> >  + num_demux_regs()
> > -+ walk_sys_regs(vcpu, (u64 __user *)NULL);
> > ++ walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
> > +ARRAY_SIZE(sys_reg_descs))
> > ++ walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
> > +ARRAY_SIZE(ptrauth_reg_descs));
> >  }
> >
> >  int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> > @@ -2541,7 +2554,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
> >  uindices++;
> >  }
> >
> > -err = walk_sys_regs(vcpu, uindices);
> > +err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> > +if (err < 0)
> > +return err;
> > +uindices += err;
> > +
> > +err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
> >  if (err < 0)
> >  return err;
> >  uindices += err;
> > @@ -2575,6 +2593,7 @@ void kvm_sys_reg_table_init(void)
> >  BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
> >  BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
> >  BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
> > +BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
> >
> >  /* We abuse the reset function to overwrite the table itself. */
> >  for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
> > @@ -2616,6 +2635,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
> >
> >  /* Generic chip reset first (so target could override). */
> >  reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> > +reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
> >
> >  table = get_target_table(vcpu->arch.target, true, &num);
> >  reset_sys_reg_descs(vcpu, table, num);
>
> This isn't very scalable, since we'd need to duplicate all the above
> code every time we add new system registers that are conditionally
> accessible.

Agreed, putting feature-specific checks in walk_sys_regs() is probably
best avoided.  Over time we would likely accumulate a fair number of
these checks.

> It seems that the SVE patches [1] solved this problem by adding a
> check_present() callback into struct sys_reg_desc. It probably makes
> sense to rebase onto that patch and just implement the callback for the
> ptrauth key registers as well.
>
> [1] https://lore.kernel.org/linux-arm-kernel/1547757219-19439-13-git-send-email-Dave.Martin@arm.com/

Note, I'm currently refactoring this so that enumeration through
KVM_GET_REG_LIST can be disabled independently of access to the
register.  This may not be the best approach, but for SVE I have a
feature-specific ID register to handle too (ID_AA64ZFR0_EL1), which
needs to be hidden from the enumeration but still accessible with
read-as-zero behaviour.

This changes the API a bit: I move to a .restrictions() callback which
returns flags to say what is disabled, and this callback is used in the
common code so that you don't have repeat your "feature present" check
in every accessor, as is currently the case.

I'm aiming to post a respun series in the next day or two.  The code
may of course change again after it gets reviewed...


Basing on [1] is probably a reasonable starting point, though.

Cheers
---Dave
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
Amit Daniel Kachhap Feb. 15, 2019, 4:57 a.m. UTC | #3
Hi,

On 2/13/19 11:24 PM, Dave P Martin wrote:
> On Wed, Feb 13, 2019 at 05:35:46PM +0000, Kristina Martsenko wrote:
>> On 28/01/2019 06:58, Amit Daniel Kachhap wrote:
>>> According to userspace settings, ptrauth key registers are conditionally
>>> present in guest system register list based on user specified flag
>>> KVM_ARM_VCPU_PTRAUTH.
>>>
>>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Christoffer Dall <christoffer.dall@arm.com>
>>> Cc: Marc Zyngier <marc.zyngier@arm.com>
>>> Cc: Kristina Martsenko <kristina.martsenko@arm.com>
>>> Cc: kvmarm@lists.cs.columbia.edu
>>> Cc: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> ---
>>>   Documentation/arm64/pointer-authentication.txt |  3 ++
>>>   arch/arm64/kvm/sys_regs.c                      | 42 +++++++++++++++++++-------
>>>   2 files changed, 34 insertions(+), 11 deletions(-)
>>>
> 
> [...]
> 
>>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> 
> [...]
> 
>>> @@ -2487,18 +2493,22 @@ static int walk_one_sys_reg(const struct sys_reg_desc *rd,
>>>   }
>>>   
>>>   /* Assumed ordered tables, see kvm_sys_reg_table_init. */
>>> -static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
>>> +static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
>>> +			const struct sys_reg_desc *desc, unsigned int len)
>>>   {
>>>   	const struct sys_reg_desc *i1, *i2, *end1, *end2;
>>>   	unsigned int total = 0;
>>>   	size_t num;
>>>   	int err;
>>>   
>>> +	if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
>>> +		return total;
>>> +
>>>   	/* We check for duplicates here, to allow arch-specific overrides. */
>>>   	i1 = get_target_table(vcpu->arch.target, true, &num);
>>>   	end1 = i1 + num;
>>> -	i2 = sys_reg_descs;
>>> -	end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
>>> +	i2 = desc;
>>> +	end2 = desc + len;
>>>   
>>>   	BUG_ON(i1 == end1 || i2 == end2);
>>>   
>>> @@ -2526,7 +2536,10 @@ unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
>>>   {
>>>   	return ARRAY_SIZE(invariant_sys_regs)
>>>   		+ num_demux_regs()
>>> -		+ walk_sys_regs(vcpu, (u64 __user *)NULL);
>>> +		+ walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
>>> +				ARRAY_SIZE(sys_reg_descs))
>>> +		+ walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
>>> +				ARRAY_SIZE(ptrauth_reg_descs));
>>>   }
>>>   
>>>   int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>>> @@ -2541,7 +2554,12 @@ int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
>>>   		uindices++;
>>>   	}
>>>   
>>> -	err = walk_sys_regs(vcpu, uindices);
>>> +	err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
>>> +	if (err < 0)
>>> +		return err;
>>> +	uindices += err;
>>> +
>>> +	err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>>>   	if (err < 0)
>>>   		return err;
>>>   	uindices += err;
>>> @@ -2575,6 +2593,7 @@ void kvm_sys_reg_table_init(void)
>>>   	BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
>>>   	BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
>>>   	BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
>>> +	BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
>>>   
>>>   	/* We abuse the reset function to overwrite the table itself. */
>>>   	for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
>>> @@ -2616,6 +2635,7 @@ void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
>>>   
>>>   	/* Generic chip reset first (so target could override). */
>>>   	reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
>>> +	reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
>>>   
>>>   	table = get_target_table(vcpu->arch.target, true, &num);
>>>   	reset_sys_reg_descs(vcpu, table, num);
>>
>> This isn't very scalable, since we'd need to duplicate all the above
>> code every time we add new system registers that are conditionally
>> accessible.
> 
> Agreed, putting feature-specific checks in walk_sys_regs() is probably
> best avoided.  Over time we would likely accumulate a fair number of
> these checks.
> 
>> It seems that the SVE patches [1] solved this problem by adding a
>> check_present() callback into struct sys_reg_desc. It probably makes
>> sense to rebase onto that patch and just implement the callback for the
>> ptrauth key registers as well.
>>
>> [1] https://lore.kernel.org/linux-arm-kernel/1547757219-19439-13-git-send-email-Dave.Martin@arm.com/
> 
> Note, I'm currently refactoring this so that enumeration through
> KVM_GET_REG_LIST can be disabled independently of access to the
> register.  This may not be the best approach, but for SVE I have a
> feature-specific ID register to handle too (ID_AA64ZFR0_EL1), which
> needs to be hidden from the enumeration but still accessible with
> read-as-zero behaviour.
> 
> This changes the API a bit: I move to a .restrictions() callback which
> returns flags to say what is disabled, and this callback is used in the
> common code so that you don't have repeat your "feature present" check
> in every accessor, as is currently the case.
> 
> I'm aiming to post a respun series in the next day or two.  The code
> may of course change again after it gets reviewed...
> 
> 
> Basing on [1] is probably a reasonable starting point, though.

Thanks Kristina and Dave for this pointer. I will rebase my next 
iteration based on it.

Thanks,
Amit
> 
> Cheers
> ---Dave
>

Patch
diff mbox series

diff --git a/Documentation/arm64/pointer-authentication.txt b/Documentation/arm64/pointer-authentication.txt
index 0529a7d..3be4ee1 100644
--- a/Documentation/arm64/pointer-authentication.txt
+++ b/Documentation/arm64/pointer-authentication.txt
@@ -87,3 +87,6 @@  created by passing a flag (KVM_ARM_VCPU_PTRAUTH) requesting this feature
 to be enabled. Without this flag, pointer authentication is not enabled
 in KVM guests and attempted use of the feature will result in an UNDEFINED
 exception being injected into the guest.
+
+Additionally, when KVM_ARM_VCPU_PTRAUTH is not set then KVM will filter
+out the authentication key registers from userspace.
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 2546a65..b46a78e 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1334,12 +1334,6 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_TTBR1_EL1), access_vm_reg, reset_unknown, TTBR1_EL1 },
 	{ SYS_DESC(SYS_TCR_EL1), access_vm_reg, reset_val, TCR_EL1, 0 },
 
-	PTRAUTH_KEY(APIA),
-	PTRAUTH_KEY(APIB),
-	PTRAUTH_KEY(APDA),
-	PTRAUTH_KEY(APDB),
-	PTRAUTH_KEY(APGA),
-
 	{ SYS_DESC(SYS_AFSR0_EL1), access_vm_reg, reset_unknown, AFSR0_EL1 },
 	{ SYS_DESC(SYS_AFSR1_EL1), access_vm_reg, reset_unknown, AFSR1_EL1 },
 	{ SYS_DESC(SYS_ESR_EL1), access_vm_reg, reset_unknown, ESR_EL1 },
@@ -1491,6 +1485,14 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_FPEXC32_EL2), NULL, reset_val, FPEXC32_EL2, 0x70 },
 };
 
+static const struct sys_reg_desc ptrauth_reg_descs[] = {
+	PTRAUTH_KEY(APIA),
+	PTRAUTH_KEY(APIB),
+	PTRAUTH_KEY(APDA),
+	PTRAUTH_KEY(APDB),
+	PTRAUTH_KEY(APGA),
+};
+
 static bool trap_dbgidr(struct kvm_vcpu *vcpu,
 			struct sys_reg_params *p,
 			const struct sys_reg_desc *r)
@@ -2093,6 +2095,8 @@  static int emulate_sys_reg(struct kvm_vcpu *vcpu,
 	r = find_reg(params, table, num);
 	if (!r)
 		r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+	if (!r && kvm_arm_vcpu_ptrauth_allowed(vcpu))
+		r = find_reg(params, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
 
 	if (likely(r)) {
 		perform_access(vcpu, params, r);
@@ -2206,6 +2210,8 @@  static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
 	r = find_reg_by_id(id, &params, table, num);
 	if (!r)
 		r = find_reg(&params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+	if (!r && kvm_arm_vcpu_ptrauth_allowed(vcpu))
+		r = find_reg(&params, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
 
 	/* Not saved in the sys_reg array and not otherwise accessible? */
 	if (r && !(r->reg || r->get_user))
@@ -2487,18 +2493,22 @@  static int walk_one_sys_reg(const struct sys_reg_desc *rd,
 }
 
 /* Assumed ordered tables, see kvm_sys_reg_table_init. */
-static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind)
+static int walk_sys_regs(struct kvm_vcpu *vcpu, u64 __user *uind,
+			const struct sys_reg_desc *desc, unsigned int len)
 {
 	const struct sys_reg_desc *i1, *i2, *end1, *end2;
 	unsigned int total = 0;
 	size_t num;
 	int err;
 
+	if (desc == ptrauth_reg_descs && !kvm_arm_vcpu_ptrauth_allowed(vcpu))
+		return total;
+
 	/* We check for duplicates here, to allow arch-specific overrides. */
 	i1 = get_target_table(vcpu->arch.target, true, &num);
 	end1 = i1 + num;
-	i2 = sys_reg_descs;
-	end2 = sys_reg_descs + ARRAY_SIZE(sys_reg_descs);
+	i2 = desc;
+	end2 = desc + len;
 
 	BUG_ON(i1 == end1 || i2 == end2);
 
@@ -2526,7 +2536,10 @@  unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu)
 {
 	return ARRAY_SIZE(invariant_sys_regs)
 		+ num_demux_regs()
-		+ walk_sys_regs(vcpu, (u64 __user *)NULL);
+		+ walk_sys_regs(vcpu, (u64 __user *)NULL, sys_reg_descs,
+				ARRAY_SIZE(sys_reg_descs))
+		+ walk_sys_regs(vcpu, (u64 __user *)NULL, ptrauth_reg_descs,
+				ARRAY_SIZE(ptrauth_reg_descs));
 }
 
 int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
@@ -2541,7 +2554,12 @@  int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices)
 		uindices++;
 	}
 
-	err = walk_sys_regs(vcpu, uindices);
+	err = walk_sys_regs(vcpu, uindices, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+	if (err < 0)
+		return err;
+	uindices += err;
+
+	err = walk_sys_regs(vcpu, uindices, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
 	if (err < 0)
 		return err;
 	uindices += err;
@@ -2575,6 +2593,7 @@  void kvm_sys_reg_table_init(void)
 	BUG_ON(check_sysreg_table(cp15_regs, ARRAY_SIZE(cp15_regs)));
 	BUG_ON(check_sysreg_table(cp15_64_regs, ARRAY_SIZE(cp15_64_regs)));
 	BUG_ON(check_sysreg_table(invariant_sys_regs, ARRAY_SIZE(invariant_sys_regs)));
+	BUG_ON(check_sysreg_table(ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs)));
 
 	/* We abuse the reset function to overwrite the table itself. */
 	for (i = 0; i < ARRAY_SIZE(invariant_sys_regs); i++)
@@ -2616,6 +2635,7 @@  void kvm_reset_sys_regs(struct kvm_vcpu *vcpu)
 
 	/* Generic chip reset first (so target could override). */
 	reset_sys_reg_descs(vcpu, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
+	reset_sys_reg_descs(vcpu, ptrauth_reg_descs, ARRAY_SIZE(ptrauth_reg_descs));
 
 	table = get_target_table(vcpu->arch.target, true, &num);
 	reset_sys_reg_descs(vcpu, table, num);