Re: [PATCH 1/2] RISC-V: KVM: Add PMU event filter support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Guest can trivially bypass PMU event DENY filters by setting unmasked high bits in the event index (eidx).
- [Medium] RISC-V defines a structurally incompatible `struct kvm_pmu_event_filter` compared to x86, which causes the KVM_SET_PMU_EVENT_FILTER ioctl number to diverge and breaks cross-architecture userspace code.
- [Medium] Missing validation for the UAPI padding field prevents safe future extensions.
--

--- Patch [1]: [PATCH 1/2] RISC-V: KVM: Add PMU event filter support ---
commit 4331081d868d3aa2c618439fb21772bb580c0495
Author: Yuhang.chen <[email protected]>

RISC-V: KVM: Add PMU event filter support
    
Allow userspace to restrict which SBI PMU events a guest is permitted
to program via the new VM ioctl KVM_SET_PMU_EVENT_FILTER.  It takes a
struct kvm_pmu_event_filter whose events[] array holds SBI PMU event
indices encoded as (type << 16) | code.

> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> index 504e73305343a..da4f639fa89f8 100644
> --- a/arch/riscv/include/uapi/asm/kvm.h
> +++ b/arch/riscv/include/uapi/asm/kvm.h
[ ... ]
> @@ -396,6 +397,25 @@ struct kvm_riscv_sbi_fwft {
>  /* One single KVM irqchip, ie. the AIA */
>  #define KVM_NR_IRQCHIPS			1
>  
> +/* for KVM_CAP_PMU_EVENT_FILTER */
> +#define KVM_PMU_EVENT_ALLOW	0
> +#define KVM_PMU_EVENT_DENY	1
> +
> +/*
> + * For KVM_SET_PMU_EVENT_FILTER: restrict which SBI PMU events a guest may
> + * configure.  Each @events entry is a SBI PMU event index (type in bits
> + * 19:16, code in bits 15:0).  %KVM_PMU_EVENT_ALLOW permits only listed
> + * events; %KVM_PMU_EVENT_DENY rejects them.  Enforced at counter
> + * configuration (SBI PMU COUNTER_CFG_MATCH), not retroactively.
> + */
> +struct kvm_pmu_event_filter {
> +	__u32 action;
> +	__u32 nevents;
> +	__u32 flags;
> +	__u32 pad;
> +	__DECLARE_FLEX_ARRAY(__u64, events);
> +};

[Severity: Medium]
Since KVM_SET_PMU_EVENT_FILTER is a generic KVM ioctl, does defining
struct kvm_pmu_event_filter with a different size and layout than the existing
x86 definition break cross-architecture userspace code? 

The size difference (due to missing fields like fixed_counter_bitmap and
different padding) causes the ioctl number to evaluate to a different numeric
value on RISC-V. Could this be unified with the existing generic definition to
maintain KVM UAPI consistency?

>  #endif
>  
>  #endif /* __LINUX_KVM_RISCV_H */
> diff --git a/arch/riscv/kvm/vcpu_pmu.c b/arch/riscv/kvm/vcpu_pmu.c
> index 2025b664961c8..189650b04c37d 100644
> --- a/arch/riscv/kvm/vcpu_pmu.c
> +++ b/arch/riscv/kvm/vcpu_pmu.c
[ ... ]
> @@ -733,6 +733,29 @@ int kvm_riscv_vcpu_pmu_ctr_stop(struct kvm_vcpu *vcpu, unsigned long ctr_base,
>  	return 0;
>  }
>  
> +static bool kvm_riscv_pmu_event_allowed(struct kvm *kvm, unsigned long eidx)
> +{
> +	struct kvm_pmu_event_filter *filter;
> +	bool in_list = false;
> +	unsigned int i;
> +
> +	filter = srcu_dereference(kvm->arch.pmu_event_filter, &kvm->srcu);
> +	if (!filter)
> +		return true;
> +
> +	for (i = 0; i < filter->nevents; i++) {
> +		if ((unsigned long)filter->events[i] == eidx) {
> +			in_list = true;
> +			break;
> +		}
> +	}

[Severity: High]
Can a malicious guest bypass the DENY filter by setting high unmasked bits
in the event index?

In kvm_riscv_pmu_event_allowed(), the guest-controlled eidx is exact-matched
against the filter entries. If the guest provides an eidx with bits above
bit 19 set (e.g., bit 63), the equality check fails and a DENY filter will
incorrectly allow it.

Because the subsequent configuration logic masks out the high bits using
get_event_type() and get_event_code(), the blocked event is still
successfully configured. Should eidx be properly masked before it is compared
against the filter events?

> +
> +	/* ALLOW: permit only listed events; DENY: reject them. */
> +	if (filter->action == KVM_PMU_EVENT_ALLOW)
> +		return in_list;
> +	return !in_list;
> +}
> +
> diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> index a9f083feeb767..6f822c43f1569 100644
> --- a/arch/riscv/kvm/vm.c
> +++ b/arch/riscv/kvm/vm.c
[ ... ]
> @@ -265,7 +268,68 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, struct kvm_enable_cap *cap)
>  	}
>  }
>  
> +#define KVM_PMU_EVENT_FILTER_MAX_EVENTS	256
> +
> +static int kvm_riscv_vm_ioctl_set_pmu_event_filter(struct kvm *kvm,
> +						   void __user *argp)
> +{
> +	struct kvm_pmu_event_filter __user *user_filter = argp;
> +	struct kvm_pmu_event_filter *filter, tmp;
> +	size_t size;
> +	int r = 0;
> +
> +	if (copy_from_user(&tmp, user_filter, sizeof(tmp)))
> +		return -EFAULT;
> +
> +	if (tmp.action != KVM_PMU_EVENT_ALLOW &&
> +	    tmp.action != KVM_PMU_EVENT_DENY)
> +		return -EINVAL;
> +
> +	if (tmp.flags)
> +		return -EINVAL;
> +
> +	if (tmp.nevents > KVM_PMU_EVENT_FILTER_MAX_EVENTS)
> +		return -E2BIG;

[Severity: Medium]
Does this code leave the tmp.pad field unvalidated?

If userspace passes uninitialized data or garbage in the padding field,
the kernel will silently accept it because there is no check ensuring
tmp.pad is zero. Will this prevent safely repurposing the padding field
for new features or flags in the future without breaking ABI compatibility
for existing applications?

> +
> +	size = struct_size(filter, events, tmp.nevents);
> +	filter = kzalloc(size, GFP_KERNEL_ACCOUNT);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1