Re: [PATCH 1/2] KVM: arm64: vgic: Forbid invalid userspace Redistributor accesses

[Marc Zyngier]

On 2020-11-16 13:09, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/11/16 1:04, Marc Zyngier wrote:
> > Hi Zenghui,
> >
> > On 2020-11-13 14:28, Zenghui Yu wrote:
> > > It's expected that users will access registers in the redistributor 
> > > *if*
> > > the RD has been initialized properly. Unfortunately userspace can be 
> > > bogus
> > > enough to access registers before setting the RD base address, and 
> > > KVM
> > > implicitly allows it (we handle the access anyway, regardless of 
> > > whether
> > > the base address is set).
> > >
> > > Bad thing happens when we're handling the user read of GICR_TYPER. We 
> > > end
> > > up with an oops when deferencing the unset rdreg...
> > >
> > >     gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
> > >             (rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
> > >
> > > Fix this issue by informing userspace what had gone wrong (-ENXIO).
> >
> > I'm worried about the "implicit" aspect of the access that this patch
> > now forbids.
> >
> > The problem is that the existing documentation doesn't cover this 
> > case, > and -ENXIO's "Getting or setting this register is not yet 
> > supported"
> > is way too vague.
>
> Indeed. How about changing to
>
>     -ENXIO  Getting or setting this register is not yet supported
>             or VGIC not properly configured (e.g., [Re]Distributor base
>             address is unknown)

Looks OK to me.

> > There is a precedent with the ITS, but that's undocumented
> > as well. Also, how about v2? If that's the wasy we are going to fix 
> > this,
> > we also nned to beef up the documentation.
>
> Sure, I plan to do so and hope it won't break the existing userspace.

Well, at this stage we can only hope.

> > Of course, the other horrible way to address the issue is to return a 
> > value
> > that doesn't have the Last bit set, since we can't synthetise it. It 
> > doesn't
> > change the userspace API, and I can even find some (admittedly  
> > twisted)
> > logic to it (since there is no base address, there is no last RD...).
>
> I'm fine with it. But I'm afraid that there might be other issues due 
> to
> the "unexpected" accesses since I haven't tested with all registers 
> from
> userspace.

I have had a look at the weekend, and couldn't see any other other GICR
register that would suffer from rdreg being NULL. I haven't looked at
GICD, but I don't anticipate anything bad on that front.

> My take is that only if the "[Re]Distributor base address" is specified
> in the system memory map, will the user-provided kvm_device_attr.offset
> make sense. And we can then handle the access to the register which is
> defined by "base address + offset".

I'd tend to agree, but it is just that this is a large change at -rc4.
I'd rather have a quick fix for 5.10, and a more invasive change for 
5.11,
spanning all the possible vgic devices.

        M.
--
Jazz is not dead. It just smells funny...