On Wed, Aug 18, 2021 at 2:45 PM Ricardo Koller <ricarkol@xxxxxxxxxx> wrote: > > On Wed, Aug 18, 2021 at 02:34:03PM -0700, Oliver Upton wrote: > > Hi Ricardo, > > > > On Wed, Aug 18, 2021 at 2:32 PM Ricardo Koller <ricarkol@xxxxxxxxxx> wrote: > > > > > > vgic_get_irq(intid) is used all over the vgic code in order to get a > > > reference to a struct irq. It warns whenever intid is not a valid number > > > (like when it's a reserved IRQ number). The issue is that this warning > > > can be triggered from userspace (e.g., KVM_IRQ_LINE for intid 1020). > > > > > > Drop the WARN call from vgic_get_irq. > > > > > > Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx> > > > --- > > > arch/arm64/kvm/vgic/vgic.c | 1 - > > > 1 file changed, 1 deletion(-) > > > > > > diff --git a/arch/arm64/kvm/vgic/vgic.c b/arch/arm64/kvm/vgic/vgic.c > > > index 111bff47e471..81cec508d413 100644 > > > --- a/arch/arm64/kvm/vgic/vgic.c > > > +++ b/arch/arm64/kvm/vgic/vgic.c > > > @@ -106,7 +106,6 @@ struct vgic_irq *vgic_get_irq(struct kvm *kvm, struct kvm_vcpu *vcpu, > > > if (intid >= VGIC_MIN_LPI) > > > return vgic_get_lpi(kvm, intid); > > > > > > - WARN(1, "Looking up struct vgic_irq for reserved INTID"); > > > > Could we maybe downgrade the message to WARN_ONCE() (to get a stack) > > or pr_warn_ratelimited()? I agree it is problematic that userspace can > > cause this WARN to fire, but it'd be helpful for debugging too. > > > > Was thinking about that, until I found this in bug.h: > > /* > * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report > * significant kernel issues that need prompt attention if they should ever > * appear at runtime. > * > * Do not use these macros when checking for invalid external inputs > * (e.g. invalid system call arguments, or invalid data coming from > * network/devices), > > Just in case, KVM_IRQ_LINE returns -EINVAL for an invalid intid (like > 1020). I think it's more appropriate for the vmm to log it. What do you > think? vgic_get_irq() is called in a bunch of other places though, right? IOW, intid doesn't necessarily come from userspace. In fact, I believe KVM_IRQ_LINE is the only place we pass a value from userspace to vgic_get_irq() (don't quote me on that). Perhaps instead the fix could be to explicitly check that the intid from userspace is valid and exit early rather than count on vgic_get_irq() to do the right thing. -- Thanks, Oliver > > > return NULL; > > > } > > > > > > -- > > > 2.33.0.rc2.250.ged5fa647cd-goog > > >
