[v3] vpci: honor read-only devices

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

Posted by Roger Pau Monne 6 years, 5 months ago

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@citrix.com>
---
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
 
 /* Dummy native helpers. Writes are ignored, reads return 1's. */
 #define pci_conf_read8(...)     0xff
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 758d9420e7..cbd1bac7fc 100644
--- 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.
-- 
2.22.0


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

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

Posted by Jan Beulich 6 years, 5 months ago

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@citrix.com>
> ---
> 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@suse.com>

Jan

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

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

Posted by Roger Pau Monné 6 years, 5 months ago

On Tue, Sep 03, 2019 at 02:16:52PM +0200, Jan Beulich wrote:
> 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@citrix.com>
> > ---
> > 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.)

One option would be to turn test_bit into assert(0) which should work
for the current usage, since test_bit shouldn't be called given the
current code and will trigger if it actually gets used. Would you be
fine with merging the chunk below into the current patch?

I would like to avoid including xc_bitops.h, since the xenpaging
Makefile already contains a comment regarding the wrong usage of libxc
internals.

> > --- 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@suse.com>

Thanks.

---8<---
diff --git a/tools/tests/vpci/emul.h b/tools/tests/vpci/emul.h
index 2e1d3057c9..796797fdc2 100644
--- a/tools/tests/vpci/emul.h
+++ b/tools/tests/vpci/emul.h
@@ -94,7 +94,7 @@ typedef union {
 #define pci_get_pdev_by_domain(...) &test_pdev
 #define pci_get_ro_map(...) NULL
 
-#define test_bit(...) false
+#define test_bit(...) ({ assert(0); false; })
 
 /* Dummy native helpers. Writes are ignored, reads return 1's. */
 #define pci_conf_read8(...)     0xff

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

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

Posted by Jan Beulich 6 years, 5 months ago

On 03.09.2019 14:51, Roger Pau Monné  wrote:
> On Tue, Sep 03, 2019 at 02:16:52PM +0200, Jan Beulich wrote:
>> 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@citrix.com>
>>> ---
>>> 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.)
> 
> One option would be to turn test_bit into assert(0) which should work
> for the current usage, since test_bit shouldn't be called given the
> current code and will trigger if it actually gets used. Would you be
> fine with merging the chunk below into the current patch?

That's marginally better, but not enough for my taste. IIRC under
tools/ we can't rely on DCE, and hence declaring (but not defining)
test_bit() isn't an option either.

Anyway, as said, I won't object to whatever the tool stack
maintainers are willing to give an ack for.

> I would like to avoid including xc_bitops.h, since the xenpaging
> Makefile already contains a comment regarding the wrong usage of libxc
> internals.

Right, and I wouldn't have dared to suggest this for something that
isn't just a test binary.

Jan

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