diff mbox series

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

Message ID 20240522225927.77398-2-stewart.hildebrand@amd.com (mailing list archive)
State New, archived
Headers show
Series PCI devices passthrough on Arm, part 3 | expand

Commit Message

Stewart Hildebrand May 22, 2024, 10:59 p.m. UTC 
From: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>

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.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v14->v15:
* re-order so this patch comes before ("xen/arm: translate virtual PCI
  bus topology for guests")
* s/access_mask/invalid/
* add U suffix to 1
* s/uint8_t/unsigned int/
* s/uint64_t/register_t/
* although Julien gave an Acked-by on v14, I omitted it due to the
  changes made in v15

v9->10:
* New patch in v10.
---
 xen/arch/arm/vpci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Stewart Hildebrand May 22, 2024, 11:04 p.m. UTC | #1 
On 5/22/24 18:59, Stewart Hildebrand wrote:
> From: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> 
> 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.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

I forgot to pick up Julien's ack [0].

[0] https://lore.kernel.org/xen-devel/8fa02e06-d8dc-4e73-a58e-e4d84b090ea8@xen.org/

> ---
> v14->v15:
> * re-order so this patch comes before ("xen/arm: translate virtual PCI
>   bus topology for guests")
> * s/access_mask/invalid/
> * add U suffix to 1
> * s/uint8_t/unsigned int/
> * s/uint64_t/register_t/
> * although Julien gave an Acked-by on v14, I omitted it due to the
>   changes made in v15
> 
> v9->10:
> * New patch in v10.
> ---
>  xen/arch/arm/vpci.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
> index 3bc4bb55082a..b63a356bb4a8 100644
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -29,6 +29,8 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>  {
>      struct pci_host_bridge *bridge = p;
>      pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
> +    const unsigned int access_size = (1U << info->dabt.size) * 8;
> +    const register_t invalid = GENMASK_ULL(access_size - 1, 0);
>      /* data is needed to prevent a pointer cast on 32bit */
>      unsigned long data;
>  
> @@ -39,7 +41,7 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
>          return 1;
>      }
>  
> -    *r = ~0ul;
> +    *r = invalid;
>  
>      return 0;
>  }
Roger Pau Monné May 23, 2024, 7:55 a.m. UTC | #2 
On Wed, May 22, 2024 at 06:59:20PM -0400, Stewart Hildebrand wrote:
> From: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> 
> 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?

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);

     set_user_reg(regs, dabt.reg, r);

(not even build tested)

Thanks, Roger.
Julien Grall May 27, 2024, 9:14 p.m. UTC | #3 
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@epam.com>
>>
>> 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.

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...

> 
> 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().

Cheers,
Roger Pau Monné May 28, 2024, 7:11 a.m. UTC | #4 
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@epam.com>
> > > 
> > > 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.
Julien Grall May 28, 2024, 9:18 a.m. UTC | #5 
Hi Roger,

On 28/05/2024 08:11, Roger Pau Monné wrote:
> 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@epam.com>
>>>>
>>>> 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.

I will let the other Arm folks commenting on it.

> 
> 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.
That would be a good idea.

> 
>>>
>>> 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.

If we decide to do a general truncation, then I would rather prefer if 
it happens outside of sign_extend(). Or the function needs to be renamed 
(I can't find a good name so far).

Cheers,
diff mbox series

Patch

diff --git a/xen/arch/arm/vpci.c b/xen/arch/arm/vpci.c
index 3bc4bb55082a..b63a356bb4a8 100644
--- a/xen/arch/arm/vpci.c
+++ b/xen/arch/arm/vpci.c
@@ -29,6 +29,8 @@  static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
 {
     struct pci_host_bridge *bridge = p;
     pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
+    const unsigned int access_size = (1U << info->dabt.size) * 8;
+    const register_t invalid = GENMASK_ULL(access_size - 1, 0);
     /* data is needed to prevent a pointer cast on 32bit */
     unsigned long data;
 
@@ -39,7 +41,7 @@  static int vpci_mmio_read(struct vcpu *v, mmio_info_t *info,
         return 1;
     }
 
-    *r = ~0ul;
+    *r = invalid;
 
     return 0;
 }