Re: [Xen-devel] [PATCH v3] vpci: honor read-only devices

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.09.2019 12:14, Roger Pau Monne wrote:
> Don't allow the hardware domain write access the PCI config space of
> devices marked as read-only.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v2:
>  - Fix test harness.
>  - Do the RO check before the ownership one.
> 
> Changes since v1:
>  - Change the approach and allow full read access, while limiting
>    write access to devices marked RO.
> ---
>  tools/tests/vpci/emul.h | 3 +++
>  xen/drivers/vpci/vpci.c | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
> index 5d47544bf7..2e1d3057c9 100644
> --- a/tools/tests/vpci/emul.h
> +++ b/tools/tests/vpci/emul.h
> @@ -92,6 +92,9 @@ typedef union {
>  #define xfree(p) free(p)
>  
>  #define pci_get_pdev_by_domain(...) &test_pdev
> +#define pci_get_ro_map(...) NULL
> +
> +#define test_bit(...) false

The latter seems rather dangerous to me, as a further addition of
test_bit() would silently build fine, but possibly produce a non-
working binary. But you're the defacto maintainer of this
harness, so if you believe it's fine so be it. (If even
xenpaging is considered "fine" to include xc_bitops.h, I wonder
if this harness couldn't do so too. And then there are three
test_bit() definitions overall under tools/ - I wonder if those
couldn't be consolidated into a single, universally usable one.)

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -411,6 +411,7 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size,
>      const struct pci_dev *pdev;
>      const struct vpci_register *r;
>      unsigned int data_offset = 0;
> +    const unsigned long *ro_map = pci_get_ro_map(sbdf.seg);
>  
>      if ( !size )
>      {
> @@ -418,6 +419,10 @@ void vpci_write(pci_sbdf_t sbdf, unsigned int reg, 
> unsigned int size,
>          return;
>      }
>  
> +    if ( ro_map && test_bit(sbdf.bdf, ro_map) )
> +        /* Ignore writes to read-only devices. */
> +        return;
> +
>      /*
>       * Find the PCI dev matching the address.
>       * Passthrough everything that's not trapped.
> 

This part
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel