| Message ID | 20190903101428.3543-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] vpci: honor read-only devices | expand |
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
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
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
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.
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(+)