| Message ID | 20211125110251.2877218-15-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI devices passthrough on Arm, part 3 | expand |
On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > For unprivileged guests vpci_{read|write} need to be re-worked > to not passthrough accesses to the registers not explicitly handled > by the corresponding vPCI handlers: without fixing that passthrough > to guests is completely unsafe as Xen allows them full access to > the registers. > > Xen needs to be sure that every register a guest accesses is not > going to cause the system to malfunction, so Xen needs to keep a > list of the registers it is safe for a guest to access. > > For example, we should only expose the PCI capabilities that we know > are safe for a guest to use, i.e.: MSI and MSI-X initially. > The rest of the capabilities should be blocked from guest access, > unless we audit them and declare safe for a guest to access. > > As a reference we might want to look at the approach currently used > by QEMU in order to do PCI passthrough. A very limited set of PCI > capabilities known to be safe for untrusted access are exposed to the > guest and registers need to be explicitly handled or else access is > rejected. Xen needs a fairly similar model in vPCI or else none of > this will be safe for unprivileged access. > > Add the corresponding TODO comment to highlight there is a problem that > needs to be fixed. > > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > Suggested-by: Jan Beulich <jbeulich@suse.com> > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> Looks okay to me in principle, but imo needs to come earlier in the series, before things actually get exposed to DomU-s. Jan
On 25.11.21 13:17, Jan Beulich wrote: > On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> For unprivileged guests vpci_{read|write} need to be re-worked >> to not passthrough accesses to the registers not explicitly handled >> by the corresponding vPCI handlers: without fixing that passthrough >> to guests is completely unsafe as Xen allows them full access to >> the registers. >> >> Xen needs to be sure that every register a guest accesses is not >> going to cause the system to malfunction, so Xen needs to keep a >> list of the registers it is safe for a guest to access. >> >> For example, we should only expose the PCI capabilities that we know >> are safe for a guest to use, i.e.: MSI and MSI-X initially. >> The rest of the capabilities should be blocked from guest access, >> unless we audit them and declare safe for a guest to access. >> >> As a reference we might want to look at the approach currently used >> by QEMU in order to do PCI passthrough. A very limited set of PCI >> capabilities known to be safe for untrusted access are exposed to the >> guest and registers need to be explicitly handled or else access is >> rejected. Xen needs a fairly similar model in vPCI or else none of >> this will be safe for unprivileged access. >> >> Add the corresponding TODO comment to highlight there is a problem that >> needs to be fixed. >> >> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> >> Suggested-by: Jan Beulich <jbeulich@suse.com> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > Looks okay to me in principle, Thanks Roger for writing most of the text in e-mails while discussing the issue > but imo needs to come earlier in the > series, before things actually get exposed to DomU-s. I can have it after "[PATCH v5 05/14] vpci: add hooks for PCI device assign/de-assign" > Jan > Thank you, Oleksandr
On Thu, Nov 25, 2021 at 12:17:32PM +0100, Jan Beulich wrote: > On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > > > For unprivileged guests vpci_{read|write} need to be re-worked > > to not passthrough accesses to the registers not explicitly handled > > by the corresponding vPCI handlers: without fixing that passthrough > > to guests is completely unsafe as Xen allows them full access to > > the registers. > > > > Xen needs to be sure that every register a guest accesses is not > > going to cause the system to malfunction, so Xen needs to keep a > > list of the registers it is safe for a guest to access. > > > > For example, we should only expose the PCI capabilities that we know > > are safe for a guest to use, i.e.: MSI and MSI-X initially. > > The rest of the capabilities should be blocked from guest access, > > unless we audit them and declare safe for a guest to access. > > > > As a reference we might want to look at the approach currently used > > by QEMU in order to do PCI passthrough. A very limited set of PCI > > capabilities known to be safe for untrusted access are exposed to the > > guest and registers need to be explicitly handled or else access is > > rejected. Xen needs a fairly similar model in vPCI or else none of > > this will be safe for unprivileged access. > > > > Add the corresponding TODO comment to highlight there is a problem that > > needs to be fixed. > > > > Suggested-by: Roger Pau Monné <roger.pau@citrix.com> > > Suggested-by: Jan Beulich <jbeulich@suse.com> > > Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> > > Looks okay to me in principle, but imo needs to come earlier in the > series, before things actually get exposed to DomU-s. Are domUs really allowed to use this code? Maybe it's done in a separate series, but has_vpci is hardcoded to false on Arm, and X86_EMU_VPCI can only be set for the hardware domain on x86. Roger.
On 13.01.2022 14:27, Roger Pau Monné wrote: > On Thu, Nov 25, 2021 at 12:17:32PM +0100, Jan Beulich wrote: >> On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: >>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> >>> For unprivileged guests vpci_{read|write} need to be re-worked >>> to not passthrough accesses to the registers not explicitly handled >>> by the corresponding vPCI handlers: without fixing that passthrough >>> to guests is completely unsafe as Xen allows them full access to >>> the registers. >>> >>> Xen needs to be sure that every register a guest accesses is not >>> going to cause the system to malfunction, so Xen needs to keep a >>> list of the registers it is safe for a guest to access. >>> >>> For example, we should only expose the PCI capabilities that we know >>> are safe for a guest to use, i.e.: MSI and MSI-X initially. >>> The rest of the capabilities should be blocked from guest access, >>> unless we audit them and declare safe for a guest to access. >>> >>> As a reference we might want to look at the approach currently used >>> by QEMU in order to do PCI passthrough. A very limited set of PCI >>> capabilities known to be safe for untrusted access are exposed to the >>> guest and registers need to be explicitly handled or else access is >>> rejected. Xen needs a fairly similar model in vPCI or else none of >>> this will be safe for unprivileged access. >>> >>> Add the corresponding TODO comment to highlight there is a problem that >>> needs to be fixed. >>> >>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> >>> Suggested-by: Jan Beulich <jbeulich@suse.com> >>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >> >> Looks okay to me in principle, but imo needs to come earlier in the >> series, before things actually get exposed to DomU-s. > > Are domUs really allowed to use this code? Maybe it's done in a > separate series, but has_vpci is hardcoded to false on Arm, and > X86_EMU_VPCI can only be set for the hardware domain on x86. I'm not sure either. This series gives the impression of exposing things, but I admit I didn't pay attention to has_vpci() being hardcoded on Arm. Then again there were at least 3 series in parallel originally, with interdependencies (iirc) not properly spelled out ... Jan
Hello, Roger, Jan! On 13.01.22 15:38, Jan Beulich wrote: > On 13.01.2022 14:27, Roger Pau Monné wrote: >> On Thu, Nov 25, 2021 at 12:17:32PM +0100, Jan Beulich wrote: >>> On 25.11.2021 12:02, Oleksandr Andrushchenko wrote: >>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>>> >>>> For unprivileged guests vpci_{read|write} need to be re-worked >>>> to not passthrough accesses to the registers not explicitly handled >>>> by the corresponding vPCI handlers: without fixing that passthrough >>>> to guests is completely unsafe as Xen allows them full access to >>>> the registers. >>>> >>>> Xen needs to be sure that every register a guest accesses is not >>>> going to cause the system to malfunction, so Xen needs to keep a >>>> list of the registers it is safe for a guest to access. >>>> >>>> For example, we should only expose the PCI capabilities that we know >>>> are safe for a guest to use, i.e.: MSI and MSI-X initially. >>>> The rest of the capabilities should be blocked from guest access, >>>> unless we audit them and declare safe for a guest to access. >>>> >>>> As a reference we might want to look at the approach currently used >>>> by QEMU in order to do PCI passthrough. A very limited set of PCI >>>> capabilities known to be safe for untrusted access are exposed to the >>>> guest and registers need to be explicitly handled or else access is >>>> rejected. Xen needs a fairly similar model in vPCI or else none of >>>> this will be safe for unprivileged access. >>>> >>>> Add the corresponding TODO comment to highlight there is a problem that >>>> needs to be fixed. >>>> >>>> Suggested-by: Roger Pau Monné <roger.pau@citrix.com> >>>> Suggested-by: Jan Beulich <jbeulich@suse.com> >>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com> >>> Looks okay to me in principle, but imo needs to come earlier in the >>> series, before things actually get exposed to DomU-s. >> Are domUs really allowed to use this code? Maybe it's done in a >> separate series, but has_vpci is hardcoded to false on Arm, and >> X86_EMU_VPCI can only be set for the hardware domain on x86. That is by intention: we do not want to have this enabled on Arm until it can really be used... > I'm not sure either. This series gives the impression of exposing things, > but I admit I didn't pay attention to has_vpci() being hardcoded on Arm. ...so we enable vPCI on Arm right after we are all set > Then again there were at least 3 series in parallel originally, with > interdependencies (iirc) not properly spelled out ... Sorry about that, we should have said that explicitly > > Jan > Thank you, Oleksandr
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c index bdc8c63f73fa..4fb77d08825a 100644 --- a/xen/drivers/vpci/vpci.c +++ b/xen/drivers/vpci/vpci.c @@ -493,6 +493,29 @@ uint32_t vpci_read(pci_sbdf_t sbdf, unsigned int reg, unsigned int size) if ( !pdev->vpci ) { spin_unlock(&pdev->vpci_lock); + /* + * TODO: for unprivileged guests vpci_{read|write} need to be re-worked + * to not passthrough accesses to the registers not explicitly handled + * by the corresponding vPCI handlers: without fixing that passthrough + * to guests is completely unsafe as Xen allows them full access to + * the registers. + * + * Xen needs to be sure that every register a guest accesses is not + * going to cause the system to malfunction, so Xen needs to keep a + * list of the registers it is safe for a guest to access. + * + * For example, we should only expose the PCI capabilities that we know + * are safe for a guest to use, i.e.: MSI and MSI-X initially. + * The rest of the capabilities should be blocked from guest access, + * unless we audit them and declare safe for a guest to access. + * + * As a reference we might want to look at the approach currently used + * by QEMU in order to do PCI passthrough. A very limited set of PCI + * capabilities known to be safe for untrusted access are exposed to the + * guest and registers need to be explicitly handled or else access is + * rejected. Xen needs a fairly similar model in vPCI or else none of + * this will be safe for unprivileged access. + */ return vpci_read_hw(sbdf, reg, size); }