| Message ID | 1559229194-3036-6-git-send-email-Dave.Martin@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Pointer Authentication and SVE support | expand |
On Thu, 30 May 2019 16:13:10 +0100 Dave Martin <Dave.Martin@arm.com> wrote: > From: Amit Daniel Kachhap <amit.kachhap@arm.com> > > This patch adds a runtime capabality for KVM tool to enable Arm64 8.3 > Pointer Authentication in guest kernel. Two vcpu features > KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable > Pointer Authentication in KVM guest after checking the capability. > > Command line options --enable-ptrauth and --disable-ptrauth are added > to use this feature. However, if those options are not provided then > also this feature is enabled if host supports this capability. I don't really get the purpose of two options, I think that's quite confusing. Should the first one either be dropped at all or called something with "force"? I guess the idea is to fail if pointer auth isn't available, but the option is supplied? Or maybe have one option with parameters? --ptrauth[,=enable,=disable] > The macros defined in the headers are not in sync and should be replaced > from the upstream. This is no longer true, I guess? Cheers, Andre. > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > Signed-off-by: Dave Martin <Dave.Martin@arm.com> [merge new kernel heaers] > --- > arm/aarch32/include/kvm/kvm-cpu-arch.h | 2 ++ > arm/aarch64/include/kvm/kvm-config-arch.h | 6 +++++- > arm/aarch64/include/kvm/kvm-cpu-arch.h | 3 +++ > arm/include/arm-common/kvm-config-arch.h | 2 ++ > arm/kvm-cpu.c | 20 ++++++++++++++++++-- > 5 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h > index d28ea67..3ec6f03 100644 > --- a/arm/aarch32/include/kvm/kvm-cpu-arch.h > +++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h > @@ -13,4 +13,6 @@ > #define ARM_CPU_ID 0, 0, 0 > #define ARM_CPU_ID_MPIDR 5 > > +#define ARM_VCPU_PTRAUTH_FEATURE 0 > + > #endif /* KVM__KVM_CPU_ARCH_H */ > diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h > index 04be43d..0279b13 100644 > --- a/arm/aarch64/include/kvm/kvm-config-arch.h > +++ b/arm/aarch64/include/kvm/kvm-config-arch.h > @@ -8,7 +8,11 @@ > "Create PMUv3 device"), \ > OPT_U64('\0', "kaslr-seed", &(cfg)->kaslr_seed, \ > "Specify random seed for Kernel Address Space " \ > - "Layout Randomization (KASLR)"), > + "Layout Randomization (KASLR)"), \ > + OPT_BOOLEAN('\0', "enable-ptrauth", &(cfg)->enable_ptrauth, \ > + "Enables pointer authentication"), \ > + OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth, \ > + "Disables pointer authentication"), > > #include "arm-common/kvm-config-arch.h" > > diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h > index a9d8563..9fa99fb 100644 > --- a/arm/aarch64/include/kvm/kvm-cpu-arch.h > +++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h > @@ -17,4 +17,7 @@ > #define ARM_CPU_CTRL 3, 0, 1, 0 > #define ARM_CPU_CTRL_SCTLR_EL1 0 > > +#define ARM_VCPU_PTRAUTH_FEATURE ((1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS) \ > + | (1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC)) > + > #endif /* KVM__KVM_CPU_ARCH_H */ > diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h > index 5734c46..1b4287d 100644 > --- a/arm/include/arm-common/kvm-config-arch.h > +++ b/arm/include/arm-common/kvm-config-arch.h > @@ -10,6 +10,8 @@ struct kvm_config_arch { > bool aarch32_guest; > bool has_pmuv3; > u64 kaslr_seed; > + bool enable_ptrauth; > + bool disable_ptrauth; > enum irqchip_type irqchip; > u64 fw_addr; > }; > diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c > index 7780251..acd1d5f 100644 > --- a/arm/kvm-cpu.c > +++ b/arm/kvm-cpu.c > @@ -68,6 +68,18 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) > vcpu_init.features[0] |= (1UL << KVM_ARM_VCPU_PSCI_0_2); > } > > + /* Check Pointer Authentication command line arguments. */ > + if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth) > + die("Both enable-ptrauth and disable-ptrauth option cannot be present"); > + /* > + * Always enable Pointer Authentication if system supports > + * this extension unless disable-ptrauth option is present. > + */ > + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) && > + kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) && > + !kvm->cfg.arch.disable_ptrauth) > + vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE; > + > /* > * If the preferred target ioctl is successful then > * use preferred target else try each and every target type > @@ -106,8 +118,12 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) > die("Unable to find matching target"); > } > > - if (err || target->init(vcpu)) > - die("Unable to initialise vcpu"); > + if (err || target->init(vcpu)) { > + if (kvm->cfg.arch.enable_ptrauth) > + die("Unable to initialise vcpu with pointer authentication feature"); > + else > + die("Unable to initialise vcpu"); > + } > > coalesced_offset = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, > KVM_CAP_COALESCED_MMIO);
On Fri, May 31, 2019 at 06:04:16PM +0100, Andre Przywara wrote: > On Thu, 30 May 2019 16:13:10 +0100 > Dave Martin <Dave.Martin@arm.com> wrote: > > > From: Amit Daniel Kachhap <amit.kachhap@arm.com> > > > > This patch adds a runtime capabality for KVM tool to enable Arm64 8.3 > > Pointer Authentication in guest kernel. Two vcpu features > > KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable > > Pointer Authentication in KVM guest after checking the capability. > > > > Command line options --enable-ptrauth and --disable-ptrauth are added > > to use this feature. However, if those options are not provided then > > also this feature is enabled if host supports this capability. > > I don't really get the purpose of two options, I think that's quite > confusing. Should the first one either be dropped at all or called > something with "force"? > > I guess the idea is to fail if pointer auth isn't available, but the > option is supplied? > > Or maybe have one option with parameters? > --ptrauth[,=enable,=disable] So, I was following two principles here, either or both of which may be bogus: 1) There should be a way to determine whether KVM turns a given feature on or off (instead of magically defaulting to something). 2) To a first approaximation, kvmtool should allow each major KVM ABI feature to be exercised. 3) By default, kvmtool should offer the maximum feature set possible to the guest. (3) is well established, but (1) and (2) may be open to question? If we hold to both principles, it makes sense to have options functionally equivalent to what I suggested (where KVM provides the control in the first place), but there may be more convenient ways to respell the options. If we really can't decide, maybe it's better to drop the options altogether until we have a real use case. I've found the options very useful for testing and debugging on the SVE side, but I can't comment on ptrauth. Maybe someone else has a view? > > The macros defined in the headers are not in sync and should be replaced > > from the upstream. > > This is no longer true, I guess? Ah yes, that comment can go. Cheers ---Dave
@Peter, do you have an opinion on this (below) ? On Thu, May 30, 2019 at 04:13:10PM +0100, Dave Martin wrote: > From: Amit Daniel Kachhap <amit.kachhap@arm.com> > > This patch adds a runtime capabality for KVM tool to enable Arm64 8.3 > Pointer Authentication in guest kernel. Two vcpu features > KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable > Pointer Authentication in KVM guest after checking the capability. > > Command line options --enable-ptrauth and --disable-ptrauth are added > to use this feature. However, if those options are not provided then > also this feature is enabled if host supports this capability. > > The macros defined in the headers are not in sync and should be replaced > from the upstream. > > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > Signed-off-by: Dave Martin <Dave.Martin@arm.com> [merge new kernel heaers] > --- > arm/aarch32/include/kvm/kvm-cpu-arch.h | 2 ++ > arm/aarch64/include/kvm/kvm-config-arch.h | 6 +++++- > arm/aarch64/include/kvm/kvm-cpu-arch.h | 3 +++ > arm/include/arm-common/kvm-config-arch.h | 2 ++ > arm/kvm-cpu.c | 20 ++++++++++++++++++-- > 5 files changed, 30 insertions(+), 3 deletions(-) > > diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h > index d28ea67..3ec6f03 100644 > --- a/arm/aarch32/include/kvm/kvm-cpu-arch.h > +++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h > @@ -13,4 +13,6 @@ > #define ARM_CPU_ID 0, 0, 0 > #define ARM_CPU_ID_MPIDR 5 > > +#define ARM_VCPU_PTRAUTH_FEATURE 0 > + > #endif /* KVM__KVM_CPU_ARCH_H */ > diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h > index 04be43d..0279b13 100644 > --- a/arm/aarch64/include/kvm/kvm-config-arch.h > +++ b/arm/aarch64/include/kvm/kvm-config-arch.h > @@ -8,7 +8,11 @@ > "Create PMUv3 device"), \ > OPT_U64('\0', "kaslr-seed", &(cfg)->kaslr_seed, \ > "Specify random seed for Kernel Address Space " \ > - "Layout Randomization (KASLR)"), > + "Layout Randomization (KASLR)"), \ > + OPT_BOOLEAN('\0', "enable-ptrauth", &(cfg)->enable_ptrauth, \ > + "Enables pointer authentication"), \ > + OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth, \ > + "Disables pointer authentication"), > > #include "arm-common/kvm-config-arch.h" > > diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h > index a9d8563..9fa99fb 100644 > --- a/arm/aarch64/include/kvm/kvm-cpu-arch.h > +++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h > @@ -17,4 +17,7 @@ > #define ARM_CPU_CTRL 3, 0, 1, 0 > #define ARM_CPU_CTRL_SCTLR_EL1 0 > > +#define ARM_VCPU_PTRAUTH_FEATURE ((1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS) \ > + | (1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC)) > + > #endif /* KVM__KVM_CPU_ARCH_H */ > diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h > index 5734c46..1b4287d 100644 > --- a/arm/include/arm-common/kvm-config-arch.h > +++ b/arm/include/arm-common/kvm-config-arch.h > @@ -10,6 +10,8 @@ struct kvm_config_arch { > bool aarch32_guest; > bool has_pmuv3; > u64 kaslr_seed; > + bool enable_ptrauth; > + bool disable_ptrauth; > enum irqchip_type irqchip; > u64 fw_addr; > }; > diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c > index 7780251..acd1d5f 100644 > --- a/arm/kvm-cpu.c > +++ b/arm/kvm-cpu.c > @@ -68,6 +68,18 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) > vcpu_init.features[0] |= (1UL << KVM_ARM_VCPU_PSCI_0_2); > } > > + /* Check Pointer Authentication command line arguments. */ > + if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth) > + die("Both enable-ptrauth and disable-ptrauth option cannot be present"); > + /* > + * Always enable Pointer Authentication if system supports > + * this extension unless disable-ptrauth option is present. > + */ > + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) && > + kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) && > + !kvm->cfg.arch.disable_ptrauth) > + vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE; > + Does anyone recall why we need these cap and feature flags for ptrauth at all? We have a window before v5.2 where we could remove them, but we need to get a move on if so... As I understand it, the main concern here was to support migrations between nodes that have the same hardware but are running different kernel versions, thus allowing a new kernel to be deployed across a cluster without having to stop the world. Thus, a guest created on an old kernel where KVM doesn's support ptrauth wouldn't detect the caps and wouldn't set those VCPU_INIT feature bits: the VCPU_INIT feature set would be recorded in the guest metadata by userspace and used for creating vcpus on the new node when migrating, forcing ptrauth still to be hidden from the guest even if the new node's kernel supports ptrauth for KVM. Fine. However, this is equally a problem for other random CPU features, and we don't handle those at all for now: any feature that the new kernel supports and is present in the hardware will result in changed CPUID registers exposed to the guest and thus migration failures. This applies even to features that require no KVM supervision whatever. So migrating between arbitrary kernel versions doesn't work today. For that, we'd need a way for the CPUID sysregs at the destination node to values different that the default: we have no logic for that today. What problem(s) are we actually trying to solve here? Do the ptrauth caps and feature flags actually solve anything? (Note, the SVE case is different: there, the cap and VCPU_INIT flag are there to work around an ABI break, so that old userspace doesn't see impossibly-large registers through KVM_GET_ONE_REG etc.) [...] Cheers ---Dave
On Mon, 3 Jun 2019 12:23:03 +0100 Dave Martin <Dave.Martin@arm.com> wrote: Hi Dave, > On Fri, May 31, 2019 at 06:04:16PM +0100, Andre Przywara wrote: > > On Thu, 30 May 2019 16:13:10 +0100 > > Dave Martin <Dave.Martin@arm.com> wrote: > > > > > From: Amit Daniel Kachhap <amit.kachhap@arm.com> > > > > > > This patch adds a runtime capabality for KVM tool to enable Arm64 8.3 > > > Pointer Authentication in guest kernel. Two vcpu features > > > KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable > > > Pointer Authentication in KVM guest after checking the capability. > > > > > > Command line options --enable-ptrauth and --disable-ptrauth are added > > > to use this feature. However, if those options are not provided then > > > also this feature is enabled if host supports this capability. > > > > I don't really get the purpose of two options, I think that's quite > > confusing. Should the first one either be dropped at all or called > > something with "force"? > > > > I guess the idea is to fail if pointer auth isn't available, but the > > option is supplied? > > > > Or maybe have one option with parameters? > > --ptrauth[,=enable,=disable] > > So, I was following two principles here, either or both of which may be > bogus: > > 1) There should be a way to determine whether KVM turns a given feature > on or off (instead of magically defaulting to something). > > 2) To a first approaximation, kvmtool should allow each major KVM ABI > feature to be exercised. > > 3) By default, kvmtool should offer the maximum feature set possible to > the guest. > > > (3) is well established, but (1) and (2) may be open to question? > > If we hold to both principles, it makes sense to have options > functionally equivalent to what I suggested (where KVM provides the > control in the first place), but there may be more convenient ways > to respell the options. > > If we really can't decide, maybe it's better to drop the options > altogether until we have a real use case. In general I prefer the lack of a *need* for options over tuneability, but my concern is not so much exposing this knob, but more how it's done ... > I've found the options very useful for testing and debugging on the SVE > side, but I can't comment on ptrauth. Maybe someone else has a view? Given that kvmtool was designed as a hacker tool, I find it quite useful to play around with those setting. I just have my gripes with those enable/disable pair, which are two related, but actually separate options, both polluting the command line options space and also being confusing to the user. I would be much happier if we would have one option per feature and a parameter: "--ptrauth={enable,disable}". Omitting the option altogether defaults to "enabled-if-available". Specifying it will force it on or off, accompanied by an error message if either(?) if not possible. This would also remove the need for the somewhat awkward "don't enable both" check. It would also more easily allow a common parser, to be used by both ptrauth and SVE, for instance. We could even introduce an explicit "default" parameter value, just in case people want to spell this case out. What do you think about this? Cheers, Andre. > > > > The macros defined in the headers are not in sync and should be replaced > > > from the upstream. > > > > This is no longer true, I guess? > > Ah yes, that comment can go. > > Cheers > ---Dave
On Mon, Jun 03, 2019 at 12:23:03PM +0100, Dave Martin wrote: > On Fri, May 31, 2019 at 06:04:16PM +0100, Andre Przywara wrote: > > On Thu, 30 May 2019 16:13:10 +0100 > > Dave Martin <Dave.Martin@arm.com> wrote: > > > > > From: Amit Daniel Kachhap <amit.kachhap@arm.com> > > > > > > This patch adds a runtime capabality for KVM tool to enable Arm64 8.3 > > > Pointer Authentication in guest kernel. Two vcpu features > > > KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable > > > Pointer Authentication in KVM guest after checking the capability. > > > > > > Command line options --enable-ptrauth and --disable-ptrauth are added > > > to use this feature. However, if those options are not provided then > > > also this feature is enabled if host supports this capability. > > > > I don't really get the purpose of two options, I think that's quite > > confusing. Should the first one either be dropped at all or called > > something with "force"? > > > > I guess the idea is to fail if pointer auth isn't available, but the > > option is supplied? > > > > Or maybe have one option with parameters? > > --ptrauth[,=enable,=disable] > > So, I was following two principles here, either or both of which may be > bogus: > > 1) There should be a way to determine whether KVM turns a given feature > on or off (instead of magically defaulting to something). > > 2) To a first approaximation, kvmtool should allow each major KVM ABI > feature to be exercised. > > 3) By default, kvmtool should offer the maximum feature set possible to > the guest. > > > (3) is well established, but (1) and (2) may be open to question? > > If we hold to both principles, it makes sense to have options > functionally equivalent to what I suggested (where KVM provides the > control in the first place), but there may be more convenient ways > to respell the options. > > If we really can't decide, maybe it's better to drop the options > altogether until we have a real use case. > > I've found the options very useful for testing and debugging on the SVE > side, but I can't comment on ptrauth. Maybe someone else has a view? I'd prefer to drop them, to be honest. Whilst they may have been useful during SVE development, it's not clear to me that they will continue to be as useful now that things should be settling down. It's probably useful to print out any features that we've explicitly enabled (or failed to enable), but I'd stop there for the time being. Will
On Mon, Jun 03, 2019 at 03:07:06PM +0100, Will Deacon wrote: > On Mon, Jun 03, 2019 at 12:23:03PM +0100, Dave Martin wrote: > > On Fri, May 31, 2019 at 06:04:16PM +0100, Andre Przywara wrote: > > > On Thu, 30 May 2019 16:13:10 +0100 > > > Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > > From: Amit Daniel Kachhap <amit.kachhap@arm.com> > > > > > > > > This patch adds a runtime capabality for KVM tool to enable Arm64 8.3 > > > > Pointer Authentication in guest kernel. Two vcpu features > > > > KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable > > > > Pointer Authentication in KVM guest after checking the capability. > > > > > > > > Command line options --enable-ptrauth and --disable-ptrauth are added > > > > to use this feature. However, if those options are not provided then > > > > also this feature is enabled if host supports this capability. > > > > > > I don't really get the purpose of two options, I think that's quite > > > confusing. Should the first one either be dropped at all or called > > > something with "force"? > > > > > > I guess the idea is to fail if pointer auth isn't available, but the > > > option is supplied? > > > > > > Or maybe have one option with parameters? > > > --ptrauth[,=enable,=disable] > > > > So, I was following two principles here, either or both of which may be > > bogus: > > > > 1) There should be a way to determine whether KVM turns a given feature > > on or off (instead of magically defaulting to something). > > > > 2) To a first approaximation, kvmtool should allow each major KVM ABI > > feature to be exercised. > > > > 3) By default, kvmtool should offer the maximum feature set possible to > > the guest. > > > > > > (3) is well established, but (1) and (2) may be open to question? > > > > If we hold to both principles, it makes sense to have options > > functionally equivalent to what I suggested (where KVM provides the > > control in the first place), but there may be more convenient ways > > to respell the options. > > > > If we really can't decide, maybe it's better to drop the options > > altogether until we have a real use case. > > > > I've found the options very useful for testing and debugging on the SVE > > side, but I can't comment on ptrauth. Maybe someone else has a view? > > I'd prefer to drop them, to be honest. Whilst they may have been useful > during SVE development, it's not clear to me that they will continue to > be as useful now that things should be settling down. It's probably useful > to print out any features that we've explicitly enabled (or failed to > enable), but I'd stop there for the time being. I don't have a strong view on this. I'm happy to respin dropping the command line options and defaulting everthing to on: for hacking purposes, it's easy to keep a local branch. Cheers ---Dave
On Mon, Jun 03, 2019 at 03:03:48PM +0100, Andre Przywara wrote: > On Mon, 3 Jun 2019 12:23:03 +0100 > Dave Martin <Dave.Martin@arm.com> wrote: > > Hi Dave, > > > On Fri, May 31, 2019 at 06:04:16PM +0100, Andre Przywara wrote: > > > On Thu, 30 May 2019 16:13:10 +0100 > > > Dave Martin <Dave.Martin@arm.com> wrote: > > > > > > > From: Amit Daniel Kachhap <amit.kachhap@arm.com> > > > > > > > > This patch adds a runtime capabality for KVM tool to enable Arm64 8.3 > > > > Pointer Authentication in guest kernel. Two vcpu features > > > > KVM_ARM_VCPU_PTRAUTH_[ADDRESS/GENERIC] are supplied together to enable > > > > Pointer Authentication in KVM guest after checking the capability. > > > > > > > > Command line options --enable-ptrauth and --disable-ptrauth are added > > > > to use this feature. However, if those options are not provided then > > > > also this feature is enabled if host supports this capability. > > > > > > I don't really get the purpose of two options, I think that's quite > > > confusing. Should the first one either be dropped at all or called > > > something with "force"? > > > > > > I guess the idea is to fail if pointer auth isn't available, but the > > > option is supplied? > > > > > > Or maybe have one option with parameters? > > > --ptrauth[,=enable,=disable] > > > > So, I was following two principles here, either or both of which may be > > bogus: > > > > 1) There should be a way to determine whether KVM turns a given feature > > on or off (instead of magically defaulting to something). > > > > 2) To a first approaximation, kvmtool should allow each major KVM ABI > > feature to be exercised. > > > > 3) By default, kvmtool should offer the maximum feature set possible to > > the guest. > > > > > > (3) is well established, but (1) and (2) may be open to question? > > > > If we hold to both principles, it makes sense to have options > > functionally equivalent to what I suggested (where KVM provides the > > control in the first place), but there may be more convenient ways > > to respell the options. > > > > If we really can't decide, maybe it's better to drop the options > > altogether until we have a real use case. > > In general I prefer the lack of a *need* for options over tuneability, but my concern is not so much exposing this knob, but more how it's done ... > > > I've found the options very useful for testing and debugging on the SVE > > side, but I can't comment on ptrauth. Maybe someone else has a view? > > Given that kvmtool was designed as a hacker tool, I find it quite useful to play around with those setting. I just have my gripes with those enable/disable pair, which are two related, but actually separate options, both polluting the command line options space and also being confusing to the user. > I would be much happier if we would have one option per feature and a parameter: "--ptrauth={enable,disable}". Omitting the option altogether defaults to "enabled-if-available". Specifying it will force it on or off, accompanied by an error message if either(?) if not possible. This would also remove the need for the somewhat awkward "don't enable both" check. > It would also more easily allow a common parser, to be used by both ptrauth and SVE, for instance. > We could even introduce an explicit "default" parameter value, just in case people want to spell this case out. > > What do you think about this? Happy to do something like that, though it looks like the decision to drop the options altogether may preempt that... Cheers ---Dave
diff --git a/arm/aarch32/include/kvm/kvm-cpu-arch.h b/arm/aarch32/include/kvm/kvm-cpu-arch.h index d28ea67..3ec6f03 100644 --- a/arm/aarch32/include/kvm/kvm-cpu-arch.h +++ b/arm/aarch32/include/kvm/kvm-cpu-arch.h @@ -13,4 +13,6 @@ #define ARM_CPU_ID 0, 0, 0 #define ARM_CPU_ID_MPIDR 5 +#define ARM_VCPU_PTRAUTH_FEATURE 0 + #endif /* KVM__KVM_CPU_ARCH_H */ diff --git a/arm/aarch64/include/kvm/kvm-config-arch.h b/arm/aarch64/include/kvm/kvm-config-arch.h index 04be43d..0279b13 100644 --- a/arm/aarch64/include/kvm/kvm-config-arch.h +++ b/arm/aarch64/include/kvm/kvm-config-arch.h @@ -8,7 +8,11 @@ "Create PMUv3 device"), \ OPT_U64('\0', "kaslr-seed", &(cfg)->kaslr_seed, \ "Specify random seed for Kernel Address Space " \ - "Layout Randomization (KASLR)"), + "Layout Randomization (KASLR)"), \ + OPT_BOOLEAN('\0', "enable-ptrauth", &(cfg)->enable_ptrauth, \ + "Enables pointer authentication"), \ + OPT_BOOLEAN('\0', "disable-ptrauth", &(cfg)->disable_ptrauth, \ + "Disables pointer authentication"), #include "arm-common/kvm-config-arch.h" diff --git a/arm/aarch64/include/kvm/kvm-cpu-arch.h b/arm/aarch64/include/kvm/kvm-cpu-arch.h index a9d8563..9fa99fb 100644 --- a/arm/aarch64/include/kvm/kvm-cpu-arch.h +++ b/arm/aarch64/include/kvm/kvm-cpu-arch.h @@ -17,4 +17,7 @@ #define ARM_CPU_CTRL 3, 0, 1, 0 #define ARM_CPU_CTRL_SCTLR_EL1 0 +#define ARM_VCPU_PTRAUTH_FEATURE ((1UL << KVM_ARM_VCPU_PTRAUTH_ADDRESS) \ + | (1UL << KVM_ARM_VCPU_PTRAUTH_GENERIC)) + #endif /* KVM__KVM_CPU_ARCH_H */ diff --git a/arm/include/arm-common/kvm-config-arch.h b/arm/include/arm-common/kvm-config-arch.h index 5734c46..1b4287d 100644 --- a/arm/include/arm-common/kvm-config-arch.h +++ b/arm/include/arm-common/kvm-config-arch.h @@ -10,6 +10,8 @@ struct kvm_config_arch { bool aarch32_guest; bool has_pmuv3; u64 kaslr_seed; + bool enable_ptrauth; + bool disable_ptrauth; enum irqchip_type irqchip; u64 fw_addr; }; diff --git a/arm/kvm-cpu.c b/arm/kvm-cpu.c index 7780251..acd1d5f 100644 --- a/arm/kvm-cpu.c +++ b/arm/kvm-cpu.c @@ -68,6 +68,18 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) vcpu_init.features[0] |= (1UL << KVM_ARM_VCPU_PSCI_0_2); } + /* Check Pointer Authentication command line arguments. */ + if (kvm->cfg.arch.enable_ptrauth && kvm->cfg.arch.disable_ptrauth) + die("Both enable-ptrauth and disable-ptrauth option cannot be present"); + /* + * Always enable Pointer Authentication if system supports + * this extension unless disable-ptrauth option is present. + */ + if (kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_ADDRESS) && + kvm__supports_extension(kvm, KVM_CAP_ARM_PTRAUTH_GENERIC) && + !kvm->cfg.arch.disable_ptrauth) + vcpu_init.features[0] |= ARM_VCPU_PTRAUTH_FEATURE; + /* * If the preferred target ioctl is successful then * use preferred target else try each and every target type @@ -106,8 +118,12 @@ struct kvm_cpu *kvm_cpu__arch_init(struct kvm *kvm, unsigned long cpu_id) die("Unable to find matching target"); } - if (err || target->init(vcpu)) - die("Unable to initialise vcpu"); + if (err || target->init(vcpu)) { + if (kvm->cfg.arch.enable_ptrauth) + die("Unable to initialise vcpu with pointer authentication feature"); + else + die("Unable to initialise vcpu"); + } coalesced_offset = ioctl(kvm->sys_fd, KVM_CHECK_EXTENSION, KVM_CAP_COALESCED_MMIO);