On Tue, Sep 20, 2022, Aaron Lewis wrote:
> If a raw event is invalid, i.e. bits set outside the event select +
> unit mask, the event will never match the search, so it's pointless
> to have it in the list. Opt for a shorter list by removing invalid
> raw events.
Please use "impossible" instead of "invalid". While I agree they are invalid,
because this is pre-existing ABI, KVM can't treat them as invalid, i.e. can't
reject them. My initial reaction to seeing remove_invalid_raw_events() was not
exactly PG-13 :-)
Can you also call out that because this is established uABI, KVM can't outright
reject garbage?
> Signed-off-by: Aaron Lewis <aaronlewis@xxxxxxxxxx>
> Reviewed-by: Jim Mattson <jmattson@xxxxxxxxxx>
> ---
> arch/x86/kvm/pmu.c | 34 ++++++++++++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
> index 98f383789579..e7d94e6b7f28 100644
> --- a/arch/x86/kvm/pmu.c
> +++ b/arch/x86/kvm/pmu.c
> @@ -577,6 +577,38 @@ void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, u64 perf_hw_id)
> }
> EXPORT_SYMBOL_GPL(kvm_pmu_trigger_event);
>
> +static inline u64 get_event_filter_mask(void)
> +{
> + u64 event_select_mask =
> + static_call(kvm_x86_pmu_get_eventsel_event_mask)();
> +
> + return event_select_mask | ARCH_PERFMON_EVENTSEL_UMASK;
> +}
> +
> +static inline bool is_event_valid(u64 event, u64 mask)
> +{
> + return !(event & ~mask);
> +}
As a general theme, don't go crazy with short helpers that only ever have a single
user. Having to jump around to find the definition interrupts the reader and
obfuscates simple things. If the code is particularly complex/weird, then adding
a helper to abstract the concept is useful, but that's not the case here.
> +
> +static void remove_invalid_raw_events(struct kvm_pmu_event_filter *filter)
s/invalid/impossible, and drop the "raw". Making up terminology when it's not
strictly necessary is always confusing.
> +{
> + u64 raw_mask;
> + int i, j;
> +
> + if (filter->flags)
If I'm reading all this magic correctly, this is dead code because
kvm_vm_ioctl_set_pmu_event_filter() checks flags
if (tmp.flags != 0)
return -EINVAL;
and does the somehwat horrific thing of:
/* Ensure nevents can't be changed between the user copies. */
*filter = tmp;
> + return;
> +
> + raw_mask = get_event_filter_mask();
> + for (i = 0, j = 0; i < filter->nevents; i++) {
> + u64 raw_event = filter->events[i];
Meh, using a local variable is unecessary.
> +
> + if (is_event_valid(raw_event, raw_mask))
> + filter->events[j++] = raw_event;
With the helpers gone, this is simply:
if (filter->events[i] & ~kvm_pmu_ops.EVENTSEL_MASK)
continue;
filter->events[j++] = filter->events[i];
> + }
> +
> + filter->nevents = j;
> +}
> +
> int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
> {
> struct kvm_pmu_event_filter tmp, *filter;
> @@ -608,6 +640,8 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
> /* Ensure nevents can't be changed between the user copies. */
> *filter = tmp;
Eww. This is so gross. But I guess it's not really that much worse than copying
only the new bits.
Can you opportunisticaly rewrite the comment to clarify that it guards against
_all_ TOCTOU attacks on the verified data?
/* Restore the verified state to guard against TOCTOU attacks. */
*filter = tmp;
As a full diff:
---
arch/x86/kvm/pmu.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c
index d0e2c7eda65b..384cefbe20dd 100644
--- a/arch/x86/kvm/pmu.c
+++ b/arch/x86/kvm/pmu.c
@@ -574,6 +574,20 @@ void kvm_pmu_trigger_event(struct kvm_vcpu *vcpu, u64 perf_hw_id)
}
EXPORT_SYMBOL_GPL(kvm_pmu_trigger_event);
+static void remove_impossible_events(struct kvm_pmu_event_filter *filter)
+{
+ int i, j;
+
+ for (i = 0, j = 0; i < filter->nevents; i++) {
+ if (filter->events[i] & ~kvm_pmu_ops.EVENTSEL_MASK)
+ continue;
+
+ filter->events[j++] = filter->events[i];
+ }
+
+ filter->nevents = j;
+}
+
int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
{
struct kvm_pmu_event_filter tmp, *filter;
@@ -602,9 +616,11 @@ int kvm_vm_ioctl_set_pmu_event_filter(struct kvm *kvm, void __user *argp)
if (copy_from_user(filter, argp, size))
goto cleanup;
- /* Ensure nevents can't be changed between the user copies. */
+ /* Restore the verified state to guard against TOCTOU attacks. */
*filter = tmp;
+ remove_impossible_events(filter);
+
/*
* Sort the in-kernel list so that we can search it with bsearch.
*/
base-commit: a5c25b1eacf50b851badf1c5cbb618094cbdf40f
--
