Re: [PATCH v16 1/5] arm/vpci: honor access size when returning an error

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 27, 2024 at 10:14:59PM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 23/05/2024 08:55, Roger Pau Monné wrote:
> > On Wed, May 22, 2024 at 06:59:20PM -0400, Stewart Hildebrand wrote:
> > > From: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > > 
> > > Guest can try to read config space using different access sizes: 8,
> > > 16, 32, 64 bits. We need to take this into account when we are
> > > returning an error back to MMIO handler, otherwise it is possible to
> > > provide more data than requested: i.e. guest issues LDRB instruction
> > > to read one byte, but we are writing 0xFFFFFFFFFFFFFFFF in the target
> > > register.
> > 
> > Shouldn't this be taken care of in the trap handler subsystem, rather
> > than forcing each handler to ensure the returned data matches the
> > access size?
> 
> I understand how this can be useful when we return all 1s.
> 
> However, in most of the current cases, we already need to deal with the
> masking because the data is extracted from a wider field (for instance, see
> the vGIC emulation). For those handlers, I would argue it would be
> concerning/ a bug if the handler return bits above the access size.
> Although, this would only impact the guest itself.

Even if there was a bug in the handler, it would be mitigated by the
truncation done in io.c.

> So overall, this seems to be a matter of taste and I don't quite (yet) see
> the benefits to do it in io.c. Regardless that...

It's up to you really, it's all ARM code so I don't really have a
stake.  IMO it makes the handlers more complicated and fragile.

If nothing else I would at least add an ASSERT() in io.c to ensure
that the data returned from the handler matches the size constrains
you expect.

> > 
> > IOW, something like:
> > 
> > diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> > index 96c740d5636c..b7e12df85f87 100644
> > --- a/xen/arch/arm/io.c
> > +++ b/xen/arch/arm/io.c
> > @@ -37,6 +37,7 @@ static enum io_state handle_read(const struct 
> > mmio_handler *handler,
> >           return IO_ABORT;
> > 
> >       r = sign_extend(dabt, r);
> > +    r = r & GENMASK_ULL((1U << dabt.size) * 8 - 1, 0);
> 
> ... in some case we need to sign extend up to the width of the register
> (even if the access is 8-byte). So we would need to do the masking *before*
> calling sign_extend().

I would consider doing the truncation in sign_extend() if suitable,
even if that's doing more than what the function name implies.

Thanks, Roger.