Re: [PATCH v3] hw/arm/virt: KVM: Enable PAuth when supported by the host

2022-01-07 Thread Richard Henderson

On 1/7/22 7:01 AM, Marc Zyngier wrote:

@@ -1380,17 +1380,10 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error 
**errp)
  return;
  }
  
-/*

- * KVM does not support modifications to this feature.
- * We have not registered the cpu properties when KVM
- * is in use, so the user will not be able to set them.
- */
-if (!kvm_enabled()) {
-arm_cpu_pauth_finalize(cpu, _err);
-if (local_err != NULL) {
+arm_cpu_pauth_finalize(cpu, _err);
+if (local_err != NULL) {
  error_propagate(errp, local_err);
  return;
-}
  }


Indentation is still off -- error + return should be out-dented one level.

Otherwise,
Reviewed-by: Richard Henderson 


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host

2022-01-06 Thread Richard Henderson

On 1/6/22 9:29 AM, Marc Zyngier wrote:

On Thu, 06 Jan 2022 17:20:33 +,
Richard Henderson  wrote:


On 1/6/22 1:16 AM, Marc Zyngier wrote:

+static bool kvm_arm_pauth_supported(void)
+{
+return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
+kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
+}


Do we really need to have them both set to play the game?  Given that
the only thing that happens is that we disable whatever host support
exists, can we have "pauth enabled" mean whatever subset the host has?


The host will always expose either both features or none, and that's
part of the ABI. From the bit of kernel documentation located in
Documentation/virt/kvm/api.rst:


4.82 KVM_ARM_VCPU_INIT
--
[...]
  - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
for arm64 only.
Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
requested.

  - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
for arm64 only.
Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
requested.


KVM will reject the initialisation if only one of the features is
requested, so checking and enabling both makes sense to me.


Well, no, that's not what that says.  It says that *if* both host
flags are set, then both guest flags must be set or both unset.


Indeed. But KVM never returns just one flag. It only exposes both or
none.


Mm.  It does beg the question of why KVM exposes multiple bits.  If they must be tied, 
then it only serves to make the interface more complicated than necessary.  We would be 
better served to have a single bit to control all of PAuth.



r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host

2022-01-06 Thread Richard Henderson

On 1/6/22 1:16 AM, Marc Zyngier wrote:

+static bool kvm_arm_pauth_supported(void)
+{
+return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
+kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
+}


Do we really need to have them both set to play the game?  Given that
the only thing that happens is that we disable whatever host support
exists, can we have "pauth enabled" mean whatever subset the host has?


The host will always expose either both features or none, and that's
part of the ABI. From the bit of kernel documentation located in
Documentation/virt/kvm/api.rst:


4.82 KVM_ARM_VCPU_INIT
--
[...]
 - KVM_ARM_VCPU_PTRAUTH_ADDRESS: Enables Address Pointer authentication
   for arm64 only.
   Depends on KVM_CAP_ARM_PTRAUTH_ADDRESS.
   If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
   both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
   KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
   requested.

 - KVM_ARM_VCPU_PTRAUTH_GENERIC: Enables Generic Pointer authentication
   for arm64 only.
   Depends on KVM_CAP_ARM_PTRAUTH_GENERIC.
   If KVM_CAP_ARM_PTRAUTH_ADDRESS and KVM_CAP_ARM_PTRAUTH_GENERIC are
   both present, then both KVM_ARM_VCPU_PTRAUTH_ADDRESS and
   KVM_ARM_VCPU_PTRAUTH_GENERIC must be requested or neither must be
   requested.


KVM will reject the initialisation if only one of the features is
requested, so checking and enabling both makes sense to me.


Well, no, that's not what that says.  It says that *if* both host flags are set, then both 
guest flags must be set or both unset.


It's probably all academic anyway, because I can't actually imagine a vendor implementing 
ADDR and not GENERIC, but in theory we ought to be able to support a host with only ADDR.



r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2] hw/arm/virt: KVM: Enable PAuth when supported by the host

2022-01-05 Thread Richard Henderson

On 1/3/22 10:05 AM, Marc Zyngier wrote:

-/*
- * KVM does not support modifications to this feature.
- * We have not registered the cpu properties when KVM
- * is in use, so the user will not be able to set them.
- */
-if (!kvm_enabled()) {
-arm_cpu_pauth_finalize(cpu, _err);
-if (local_err != NULL) {
+   arm_cpu_pauth_finalize(cpu, _err);
+   if (local_err != NULL) {
  error_propagate(errp, local_err);
  return;
-}
-}
+   }


Looks like the indentation is off?


+static bool kvm_arm_pauth_supported(void)
+{
+return (kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_ADDRESS) &&
+kvm_check_extension(kvm_state, KVM_CAP_ARM_PTRAUTH_GENERIC));
+}


Do we really need to have them both set to play the game?  Given that the only thing that 
happens is that we disable whatever host support exists, can we have "pauth enabled" mean 
whatever subset the host has?




@@ -521,6 +527,17 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures 
*ahcf)
   */
  struct kvm_vcpu_init init = { .target = -1, };
  
+/*

+ * Ask for Pointer Authentication if supported. We can't play the
+ * SVE trick of synthetising the ID reg as KVM won't tell us


synthesizing


+ * whether we have the architected or IMPDEF version of PAuth, so
+ * we have to use the actual ID regs.
+ */
+if (kvm_arm_pauth_supported()) {
+init.features[0] |= (1 << KVM_ARM_VCPU_PTRAUTH_ADDRESS |
+1 << KVM_ARM_VCPU_PTRAUTH_GENERIC);


Align the two 1's.

Otherwise, it looks good.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v5 0/2] MTE support for KVM guest

2020-12-09 Thread Richard Henderson
On 12/9/20 12:39 PM, Catalin Marinas wrote:
>> I would have thought that the best way is to use TCO, so that we don't have 
>> to
>> have dual mappings (and however many MB of extra page tables that might 
>> imply).
> 
> The problem appears when the VMM wants to use MTE itself (e.g. linked
> against an MTE-aware glibc), toggling TCO is no longer generic enough,
> especially when it comes to device emulation.

But we do know exactly when we're manipulating guest memory -- we have special
routines for that.  So the special routines gain a toggle of TCO around the
exact guest memory manipulation, not a blanket disable of MTE across large
swaths of QEMU.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v5 0/2] MTE support for KVM guest

2020-12-09 Thread Richard Henderson
On 12/9/20 9:27 AM, Catalin Marinas wrote:
> On Wed, Dec 09, 2020 at 01:25:18PM +, Marc Zyngier wrote:
>> Would this syscall operate on the guest address space? Or on the VMM's
>> own mapping?
...
> Whatever is easier for the VMM, I don't think it matters as long as the
> host kernel can get the actual physical address (and linear map
> correspondent). Maybe simpler if it's the VMM address space as the
> kernel can check the access permissions in case you want to hide the
> guest memory from the VMM for other reasons (migration is also off the
> table).

Indeed, such a syscall is no longer specific to vmm's and may be used for any
bulk move of tags that userland might want.

> Without syscalls, an option would be for the VMM to create two mappings:
> one with PROT_MTE for migration and the other without for normal DMA
> etc. That's achievable using memfd_create() or shm_open() and two mmap()
> calls, only one having PROT_MTE. The VMM address space should be
> sufficiently large to map two guest IPAs.

I would have thought that the best way is to use TCO, so that we don't have to
have dual mappings (and however many MB of extra page tables that might imply).


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 0/2] MTE support for KVM guest

2020-09-10 Thread Richard Henderson
On 9/10/20 3:24 AM, Steven Price wrote:
> It is a shame, however I suspect this is because to use those instructions you
> need to know the block size held in GMID_EL1. And at least in theory that 
> could
> vary between CPUs.

Which is no different from having to read DCZID_EL0 in order to implement
memset, in my opinion.  But, whatever.


> When we have some real hardware it would be worth profiling this. At the 
> moment
> I've no idea whether the kernel entry overhead would make such an interface
> useful from a performance perspective or not.

Yep.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 0/2] MTE support for KVM guest

2020-09-09 Thread Richard Henderson
On 9/9/20 8:25 AM, Andrew Jones wrote:
>>  * Provide a KVM-specific method to extract the tags from guest memory.
>>This might also have benefits in terms of providing an easy way to
>>read bulk tag data from guest memory (since the LDGM instruction
>>isn't available at EL0).
> 
> Maybe we need a new version of KVM_GET_DIRTY_LOG that also provides
> the tags for all addresses of each dirty page.

KVM_GET_DIRTY_LOG just provides one bit per dirty page, no?  Then VMM copies
the data out from its local address to guest memory.

There'd be no difference with or without tags, afaik.  It's just about how VMM
copies the data, with or without tags.

>>  * Provide support for user space setting the TCMA0 or TCMA1 bits in
>>TCR_EL1. These would allow the VMM to generate pointers which are not
>>tag checked.
> 
> So this is necessary to allow the VMM to keep tag checking enabled for
> itself, plus map guest memory as PROT_MTE, and write to that memory when
> needed? 

I don't see a requirement for the VMM to set TCMA0.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v4 2/2] target/arm: kvm: Handle potential issue with dabt injection

2020-03-23 Thread Richard Henderson
On 3/23/20 4:32 AM, Beata Michalska wrote:
>  uint8_t ext_dabt_pending; /* Request for injecting ext DABT */
> +uint8_t ext_dabt_raised; /* Tracking/verifying injection of ext DABT */

Is there a reason these are uint8_t and not bool?


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 3/5] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions

2019-12-06 Thread Richard Henderson
On 12/6/19 6:08 AM, Peter Maydell wrote:
>>  DEF_HELPER_FLAGS_3(autdb, TCG_CALL_NO_WG, i64, env, i64, i64)
>>  DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64)
>>  DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
>> +
>> +DEF_HELPER_3(check_hcr_el2_trap, void, env, i32, i32)
> 
> This has to be in helper.h, not helper-a64.h, otherwise
> the arm-softmmu target won't build. helper-a64.h is for
> helper functions which only exist in the aarch64 binary.

Oh, while we're at it,

  DEF_HELPER_FLAGS_3(..., TCG_CALL_NO_WG, ...)

The helper does not modify tcg globals (on successful return).
It does read globals (via the exception path), and of course it has side
effects (the exception).


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 3/5] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions

2019-12-02 Thread Richard Henderson
On 12/2/19 4:45 PM, Marc Zyngier wrote:
>> Annoying that there's a bug in the manual -- FPSID is listed as group 0 in
>> plenty of places, except in the pseudo-code for Accessing the FPSID
>> which uses TID3.
> 
> Are you sure? I'm looking at DDI0487E_a,
...
> Or have you spotted a discrepancy
> somewhere else (which would be oh-so-surprising...)?

In DDI0487E_a, page G8-6028:

> elsif EL2Enabled() && !ELUsingAArch32(EL2) && HCR_EL2.TID3 == '1' then
>   AArch64.AArch32SystemAccessTrap(EL2, 0x08);
> elsif EL2Enabled() && ELUsingAArch32(EL2) && HCR.TID3 == '1' then
>   AArch32.TakeHypTrapException(0x08);
> else
>   return FPSID;

within the summary documentation for FPSID.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 5/5] target/arm: Add support for missing Jazelle system registers

2019-12-02 Thread Richard Henderson
On 12/1/19 12:20 PM, Marc Zyngier wrote:
> +if (cpu_isar_feature(jazelle, cpu)) {
> +ARMCPRegInfo jazelle_regs[] = {

static const.

Otherwise,
Reviewed-by: Richard Henderson 


r~

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 4/5] target/arm: Handle AArch32 CP15 trapping via HSTR_EL2

2019-12-02 Thread Richard Henderson
On 12/1/19 12:20 PM, Marc Zyngier wrote:
> +/* Check for an EL2 trap due to HSTR_EL2. We expect EL0 accesses
> + * to sysregs non accessible at EL0 to have UNDEF-ed already.
> + */

We're enforcing

/*
 * Multi-line comment
 */

for qemu now; checkpatch should be reporting on that.

> +if (!env->aarch64 && arm_current_el(env) < 2 && ri->cp == 15 &&
> +(arm_hcr_el2_eff(env) & (HCR_E2H | HCR_TGE)) != (HCR_E2H | HCR_TGE)) 
> {

Use is_a64(env) not env->aarch64 directly.

Otherwise,
Reviewed-by: Richard Henderson 


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 3/5] target/arm: Handle trapping to EL2 of AArch32 VMRS instructions

2019-12-02 Thread Richard Henderson
On 12/1/19 12:20 PM, Marc Zyngier wrote:
> HCR_EL2.TID3 requires that AArch32 reads of MVFR[012] are trapped to
> EL2, and HCR_EL2.TID0 does the same for reads of FPSID.
> In order to handle this, introduce a new TCG helper function that
> checks for these control bits before executing the VMRC instruction.
> 
> Tested with a hacked-up version of KVM/arm64 that sets the control
> bits for 32bit guests.
> 
> Reviewed-by: Edgar E. Iglesias 
> Signed-off-by: Marc Zyngier 
> ---
>  target/arm/helper-a64.h|  2 ++
>  target/arm/translate-vfp.inc.c | 18 +++---
>  target/arm/vfp_helper.c| 29 +
>  3 files changed, 46 insertions(+), 3 deletions(-)

Reviewed-by: Richard Henderson 

Annoying that there's a bug in the manual -- FPSID is listed as group 0 in
plenty of places, except in the pseudo-code for Accessing the FPSID which uses
TID3.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 2/5] target/arm: Honor HCR_EL2.TID1 trapping requirements

2019-12-02 Thread Richard Henderson
On 12/1/19 12:20 PM, Marc Zyngier wrote:
> HCR_EL2.TID1 mandates that access from EL1 to REVIDR_EL1, AIDR_EL1
> (and their 32bit equivalents) as well as TCMTR, TLBTR are trapped
> to EL2. QEMU ignores it, making it harder for a hypervisor to
> virtualize the HW (though to be fair, no known hypervisor actually
> cares).
> 
> Do the right thing by trapping to EL2 if HCR_EL2.TID1 is set.
> 
> Reviewed-by: Edgar E. Iglesias 
> Signed-off-by: Marc Zyngier 
> ---
>  target/arm/helper.c | 36 
>  1 file changed, 32 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson 


r~

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v2 1/5] target/arm: Honor HCR_EL2.TID2 trapping requirements

2019-12-02 Thread Richard Henderson
On 12/1/19 12:20 PM, Marc Zyngier wrote:
> HCR_EL2.TID2 mandates that access from EL1 to CTR_EL0, CCSIDR_EL1,
> CCSIDR2_EL1, CLIDR_EL1, CSSELR_EL1 are trapped to EL2, and QEMU
> completely ignores it, making it impossible for hypervisors to
> virtualize the cache hierarchy.
> 
> Do the right thing by trapping to EL2 if HCR_EL2.TID2 is set.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  target/arm/helper.c | 31 +++
>  1 file changed, 27 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson 


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements

2019-11-26 Thread Richard Henderson
On 11/23/19 11:56 AM, Marc Zyngier wrote:
> HCR_EL2.TID3 mandates that access from EL1 to a long list of id
> registers traps to EL2, and QEMU has so far ignored this requirement.
> 
> This breaks (among other things) KVM guests that have PtrAuth enabled,
> while the hypervisor doesn't want to expose the feature to its guest.
> To achieve this, KVM traps the ID registers (ID_AA64ISAR1_EL1 in this
> case), and masks out the unsupported feature.
> 
> QEMU not honoring the trap request means that the guest observes
> that the feature is present in the HW, starts using it, and dies
> a horrible death when KVM injects an UNDEF, because the feature
> *really* isn't supported.
> 
> Do the right thing by trapping to EL2 if HCR_EL2.TID3 is set.
> 
> Reported-by: Will Deacon 
> Signed-off-by: Marc Zyngier 
> ---
> There is a number of other trap bits missing (TID[0-2], for example),
> but this at least gets a mainline Linux going with cpu=max.

BTW, Peter, this appears to have been the bug that was causing me so many
problems on my VHE branch.  Probably *exactly* this bug wrt ptrauth, since that
would also be included with -cpu max.

I am now able to boot a kvm guest kernel to the point of the no rootfs panic,
which I wasn't before.

I can only think that I mis-identified the true cause in Lyon.

Anyway, thanks Marc!


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH] target/arm: Honor HCR_EL2.TID3 trapping requirements

2019-11-26 Thread Richard Henderson
On 11/23/19 11:56 AM, Marc Zyngier wrote:
> +static CPAccessResult access_aa64idreg(CPUARMState *env, const ARMCPRegInfo 
> *ri,
> +   bool isread)
> +{
> +if ((arm_current_el(env) < 2) && (arm_hcr_el2_eff(env) & HCR_TID3)) {
> +return CP_ACCESS_TRAP_EL2;
> +}
> +
> +return CP_ACCESS_OK;
> +}
> +

The only thing I would suggest is to call this access_aa64_tid3, because
tid{0,1,2} also need to be handled in a similar way, and we'll need little
helper functions for those too.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH] target/arm: Fix ISR_EL1 tracking when executing at EL2

2019-11-22 Thread Richard Henderson
On 11/22/19 2:16 PM, Peter Maydell wrote:
> RTH: vaguely wondering if this might be related to the
> bug you ran into trying to test your VHE emulation
> patchset...

Thanks for the thought.  It might be related, but it isn't the final cause:
the inner guest does not yet succeed including this patch.


r~

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 04/13] arm64/kvm: hide ptrauth from guests

2018-12-10 Thread Richard Henderson
On 12/10/18 2:12 PM, Kristina Martsenko wrote:
> The plan was to disable trapping, yes. However, after that thread there
> was a retrospective change applied to the architecture, such that the
> XPACLRI (and XPACD/XPACI) instructions are no longer trapped by
> HCR_EL2.API. (The public documentation on this has not been updated
> yet.) This means that no HINT-space instructions should trap anymore.

Ah, thanks for the update.  I'll update my QEMU patch set.

>> It seems like the header comment here, and
> Sorry, which header comment?

Sorry, the patch commit message.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 08/13] arm64: expose user PAC bit positions via ptrace

2018-12-10 Thread Richard Henderson
On 12/10/18 6:03 AM, Catalin Marinas wrote:
>> However, it won't be too long before someone implements support for
>> ARMv8.2-LVA, at which point, without changes to mandatory pointer tagging, we
>> will only have 3 authentication bits: [54:52].  This seems useless and easily
>> brute-force-able.
> 
> Such support is already here (about to be queued):
> 
> https://lore.kernel.org/linux-arm-kernel/20181206225042.11548-1-steve.cap...@arm.com/

Thanks for the pointer.

>> Unfortunately, there is no obvious path to making this optional that does not
>> break compatibility with Documentation/arm64/tagged-pointers.txt.
> 
> There is also the ARMv8.5 MTE (memory tagging) which relies on tagged
> pointers.

So it does.  I hadn't read through that extension completely before.

> An alternative would be to allow the opt-in to 52-bit VA, leaving it at
> 48-bit by default. However, it has the problem of changing the PAC size
> and not being able to return.

Perhaps the opt-in should be at exec time, with ELF flags (or equivalent) on
the application.  Because, as you say, changing the shape of the PAC in the
middle of execution is in general not possible.

It isn't perfect, since old kernels won't fail to exec an application setting
flags that can't be supported.  And it requires tooling changes.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 08/13] arm64: expose user PAC bit positions via ptrace

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
> When pointer authentication is in use, data/instruction pointers have a
> number of PAC bits inserted into them. The number and position of these
> bits depends on the configured TCR_ELx.TxSZ and whether tagging is
> enabled. ARMv8.3 allows tagging to differ for instruction and data
> pointers.

At this point I think it's worth starting a discussion about pointer tagging,
and how we can make it controllable and not mandatory.

With this patch set, we are enabling 7 authentication bits: [54:48].

However, it won't be too long before someone implements support for
ARMv8.2-LVA, at which point, without changes to mandatory pointer tagging, we
will only have 3 authentication bits: [54:52].  This seems useless and easily
brute-force-able.

I assume that pointer tagging is primarily used by Android, since I'm not aware
of anything else that uses it at all.

Unfortunately, there is no obvious path to making this optional that does not
break compatibility with Documentation/arm64/tagged-pointers.txt.

I've been thinking that there ought to be some sort of global setting, akin to
/proc/sys/kernel/randomize_va_space, as well as a prctl which an application
could use to selectively enable TBI/TBID for an application that actually uses
tagging.

The global /proc setting allows the default to remain 1, which would let any
application using tagging to continue working.  If there are none, the sysadmin
can set the default to 0.  Going forward, applications could be updated to use
the prctl, allowing more systems to set the default to 0.

FWIW, pointer authentication continues to work when enabling TBI, but not the
other way around.  Thus the prctl could be used to enable TBI at any point, but
if libc is built with PAuth, there's no way to turn it back off again.



r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 08/13] arm64: expose user PAC bit positions via ptrace

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
> From: Mark Rutland 
> 
> When pointer authentication is in use, data/instruction pointers have a
> number of PAC bits inserted into them. The number and position of these
> bits depends on the configured TCR_ELx.TxSZ and whether tagging is
> enabled. ARMv8.3 allows tagging to differ for instruction and data
> pointers.
> 
> For userspace debuggers to unwind the stack and/or to follow pointer
> chains, they need to be able to remove the PAC bits before attempting to
> use a pointer.
> 
> This patch adds a new structure with masks describing the location of
> the PAC bits in userspace instruction and data pointers (i.e. those
> addressable via TTBR0), which userspace can query via PTRACE_GETREGSET.
> By clearing these bits from pointers (and replacing them with the value
> of bit 55), userspace can acquire the PAC-less versions.
> 
> This new regset is exposed when the kernel is built with (user) pointer
> authentication support, and the address authentication feature is
> enabled. Otherwise, the regset is hidden.
> 
> Signed-off-by: Mark Rutland 
> Signed-off-by: Kristina Martsenko 
> Cc: Catalin Marinas 
> Cc: Ramana Radhakrishnan 
> Cc: Will Deacon 
> ---
>  arch/arm64/include/asm/pointer_auth.h |  8 
>  arch/arm64/include/uapi/asm/ptrace.h  |  7 +++
>  arch/arm64/kernel/ptrace.c| 38 
> +++
>  include/uapi/linux/elf.h  |  1 +
>  4 files changed, 54 insertions(+)

Reviewed-by: Richard Henderson 


r~

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 07/13] arm64: add basic pointer authentication support

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
> From: Mark Rutland 
> 
> This patch adds basic support for pointer authentication, allowing
> userspace to make use of APIAKey, APIBKey, APDAKey, APDBKey, and
> APGAKey. The kernel maintains key values for each process (shared by all
> threads within), which are initialised to random values at exec() time.
> 
> The ID_AA64ISAR1_EL1.{APA,API,GPA,GPI} fields are exposed to userspace,
> to describe that pointer authentication instructions are available and
> that the kernel is managing the keys. Two new hwcaps are added for the
> same reason: PACA (for address authentication) and PACG (for generic
> authentication).
> 
> Signed-off-by: Mark Rutland 
> Signed-off-by: Kristina Martsenko 
> Tested-by: Adam Wallis 
> Cc: Catalin Marinas 
> Cc: Ramana Radhakrishnan 
> Cc: Suzuki K Poulose 
> Cc: Will Deacon 
> ---
>  arch/arm64/include/asm/pointer_auth.h | 75 
> +++
>  arch/arm64/include/asm/thread_info.h  |  4 ++
>  arch/arm64/include/uapi/asm/hwcap.h   |  2 +
>  arch/arm64/kernel/cpufeature.c| 13 ++
>  arch/arm64/kernel/cpuinfo.c   |  2 +
>  arch/arm64/kernel/process.c   |  4 ++
>  6 files changed, 100 insertions(+)
>  create mode 100644 arch/arm64/include/asm/pointer_auth.h

Reviewed-by: Richard Henderson 


r~

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 06/13] arm64/cpufeature: detect pointer authentication

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
> From: Mark Rutland 
> 
> So that we can dynamically handle the presence of pointer authentication
> functionality, wire up probing code in cpufeature.c.
> 
> From ARMv8.3 onwards, ID_AA64ISAR1 is no longer entirely RES0, and now
> has four fields describing the presence of pointer authentication
> functionality:
> 
> * APA - address authentication present, using an architected algorithm
> * API - address authentication present, using an IMP DEF algorithm
> * GPA - generic authentication present, using an architected algorithm
> * GPI - generic authentication present, using an IMP DEF algorithm
> 
> This patch checks for both address and generic authentication,
> separately. It is assumed that if all CPUs support an IMP DEF algorithm,
> the same algorithm is used across all CPUs.
> 
> Signed-off-by: Mark Rutland 
> Signed-off-by: Kristina Martsenko 
> Cc: Catalin Marinas 
> Cc: Suzuki K Poulose 
> Cc: Will Deacon 
> ---
>  arch/arm64/include/asm/cpucaps.h|  8 +++-
>  arch/arm64/include/asm/cpufeature.h | 12 +
>  arch/arm64/kernel/cpufeature.c  | 90 
> +++++
>  3 files changed, 109 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 05/13] arm64: Don't trap host pointer auth use to EL2

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
> From: Mark Rutland 
> 
> To allow EL0 (and/or EL1) to use pointer authentication functionality,
> we must ensure that pointer authentication instructions and accesses to
> pointer authentication keys are not trapped to EL2.
> 
> This patch ensures that HCR_EL2 is configured appropriately when the
> kernel is booted at EL2. For non-VHE kernels we set HCR_EL2.{API,APK},
> ensuring that EL1 can access keys and permit EL0 use of instructions.
> For VHE kernels host EL0 (TGE && E2H) is unaffected by these settings,
> and it doesn't matter how we configure HCR_EL2.{API,APK}, so we don't
> bother setting them.
> 
> This does not enable support for KVM guests, since KVM manages HCR_EL2
> itself when running VMs.
> 
> Signed-off-by: Mark Rutland 
> Signed-off-by: Kristina Martsenko 
> Acked-by: Christoffer Dall 
> Cc: Catalin Marinas 
> Cc: Marc Zyngier 
> Cc: Will Deacon 
> Cc: kvmarm@lists.cs.columbia.edu
> ---
>  arch/arm64/include/asm/kvm_arm.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson 


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 04/13] arm64/kvm: hide ptrauth from guests

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
> From: Mark Rutland 
> 
> In subsequent patches we're going to expose ptrauth to the host kernel
> and userspace, but things are a bit trickier for guest kernels. For the
> time being, let's hide ptrauth from KVM guests.
> 
> Regardless of how well-behaved the guest kernel is, guest userspace
> could attempt to use ptrauth instructions, triggering a trap to EL2,
> resulting in noise from kvm_handle_unknown_ec(). So let's write up a
> handler for the PAC trap, which silently injects an UNDEF into the
> guest, as if the feature were really missing.

Reviewing the long thread that accompanied v5, I thought we were *not* going to
trap PAuth instructions from the guest.

In particular, the OS distribution may legitimately be built to include
hint-space nops.  This includes XPACLRI, which is used by the C++ exception
unwinder and not controlled by SCTLR_EL1.EnI{A,B}.

It seems like the header comment here, and

> +/*
> + * Guest usage of a ptrauth instruction (which the guest EL1 did not turn 
> into
> + * a NOP).
> + */
> +static int kvm_handle_ptrauth(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +

here, need updating.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 03/13] arm64/kvm: consistently handle host HCR_EL2 flags

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
> From: Mark Rutland 
> 
> In KVM we define the configuration of HCR_EL2 for a VHE HOST in
> HCR_HOST_VHE_FLAGS, but we don't have a similar definition for the
> non-VHE host flags, and open-code HCR_RW. Further, in head.S we
> open-code the flags for VHE and non-VHE configurations.
> 
> In future, we're going to want to configure more flags for the host, so
> lets add a HCR_HOST_NVHE_FLAGS defintion, and consistently use both
> HCR_HOST_VHE_FLAGS and HCR_HOST_NVHE_FLAGS in the kvm code and head.S.
> 
> We now use mov_q to generate the HCR_EL2 value, as we use when
> configuring other registers in head.S.
> 
> Signed-off-by: Mark Rutland 
> Signed-off-by: Kristina Martsenko 
> Reviewed-by: Christoffer Dall 
> Cc: Catalin Marinas 
> Cc: Marc Zyngier 
> Cc: Will Deacon 
> Cc: kvmarm@lists.cs.columbia.edu
> ---
>  arch/arm64/include/asm/kvm_arm.h | 1 +
>  arch/arm64/kernel/head.S | 5 ++---
>  arch/arm64/kvm/hyp/switch.c  | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson 


r~

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 01/13] arm64: add comments about EC exception levels

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
> To make it clear which exceptions can't be taken to EL1 or EL2, add
> comments next to the ESR_ELx_EC_* macro definitions.
> 
> Signed-off-by: Kristina Martsenko 
> ---
>  arch/arm64/include/asm/esr.h | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson 


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [PATCH v6 02/13] arm64: add pointer authentication register bits

2018-12-09 Thread Richard Henderson
On 12/7/18 12:39 PM, Kristina Martsenko wrote:
>  #define SCTLR_ELx_DSSBS  (1UL << 44)
> +#define SCTLR_ELx_ENIA   (1 << 31)

1U or 1UL lest you produce signed -0x8000.

Otherwise,
Reviewed-by: Richard Henderson 


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm


Re: [Qemu-devel] [RFC QEMU 1/2] arm/virt: Initialize generic timer scale factor dynamically

2018-11-08 Thread Richard Henderson
On 11/7/18 7:48 PM, Bijan Mottahedeh wrote:
>  
> +static void set_system_clock_scale(void)
> +{
> +unsigned long cntfrq_el0;
> +
> +asm volatile("mrs %0, cntfrq_el0" : "=r"(cntfrq_el0));
> +
> +if (cntfrq_el0 == 0) {
> +cntfrq_el0 = GTIMER_SCALE_DEF;
> +}
> +
> +system_clock_scale = NANOSECONDS_PER_SECOND / (int)cntfrq_el0;
> +}

This only works for kvm.

For TCG you need to use the default always.  In particular, it won't even
compile for an x86 host.


r~
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm