| Message ID | 56548D8402000078000B8840@prv-mh.provo.novell.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 24 Nov 2015, Jan Beulich wrote: > Now that the hypervisor intercepts all config space writes and monitors > changes to the masking flags, this undoes the main effect of the > XSA-129 fix, exposing the masking capability again to guests. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > TBD: We probably need to deal with running on an older hypervisor. I > can't, however, immediately see a way for qemu to find out. Actually QEMU has already an infrastructure to detect the hypervisor version at compile time, see include/hw/xen/xen_common.h. You could #define the right emu_mask depending on the hypervisor. > --- a/hw/xen/xen_pt_config_init.c > +++ b/hw/xen/xen_pt_config_init.c > @@ -1333,7 +1333,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] > .init_val = 0x0000, > .res_mask = 0xFE00, > .ro_mask = 0x018E, > - .emu_mask = 0x017E, > + .emu_mask = 0x007E, > .init = xen_pt_msgctrl_reg_init, > .u.w.read = xen_pt_word_reg_read, > .u.w.write = xen_pt_msgctrl_reg_write, > @@ -1387,8 +1387,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] > .offset = PCI_MSI_MASK_32, > .size = 4, > .init_val = 0x00000000, > - .ro_mask = 0xFFFFFFFF, > - .emu_mask = 0xFFFFFFFF, > + .ro_mask = 0x00000000, > + .emu_mask = 0x00000000, > .init = xen_pt_mask_reg_init, > .u.dw.read = xen_pt_long_reg_read, > .u.dw.write = xen_pt_long_reg_write, > @@ -1398,8 +1398,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] > .offset = PCI_MSI_MASK_64, > .size = 4, > .init_val = 0x00000000, > - .ro_mask = 0xFFFFFFFF, > - .emu_mask = 0xFFFFFFFF, > + .ro_mask = 0x00000000, > + .emu_mask = 0x00000000, > .init = xen_pt_mask_reg_init, > .u.dw.read = xen_pt_long_reg_read, > .u.dw.write = xen_pt_long_reg_write, > > > >
>>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote: > On Tue, 24 Nov 2015, Jan Beulich wrote: >> Now that the hypervisor intercepts all config space writes and monitors >> changes to the masking flags, this undoes the main effect of the >> XSA-129 fix, exposing the masking capability again to guests. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> TBD: We probably need to deal with running on an older hypervisor. I >> can't, however, immediately see a way for qemu to find out. > > Actually QEMU has already an infrastructure to detect the hypervisor > version at compile time, see include/hw/xen/xen_common.h. You could > #define the right emu_mask depending on the hypervisor. We don't want compile time detection here, but runtime one. Jan
On Mon, 7 Dec 2015, Jan Beulich wrote: > >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote: > > On Tue, 24 Nov 2015, Jan Beulich wrote: > >> Now that the hypervisor intercepts all config space writes and monitors > >> changes to the masking flags, this undoes the main effect of the > >> XSA-129 fix, exposing the masking capability again to guests. Could you please mention the relevant commit ids in Xen? What happens if QEMU, with this change, is running on top of an older Xen that doesn't intercepts all config space writes? > >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> TBD: We probably need to deal with running on an older hypervisor. I > >> can't, however, immediately see a way for qemu to find out. > > > > Actually QEMU has already an infrastructure to detect the hypervisor > > version at compile time, see include/hw/xen/xen_common.h. You could > > #define the right emu_mask depending on the hypervisor. > > We don't want compile time detection here, but runtime one. I guess the issue is that a fix was backported to Xen that changed its behaviour in past releases, right? Is there a way to detect the presence of this fix in Xen, by invoking an hypercall and checking the returned values and error numbers?
>>> On 07.12.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote: > On Mon, 7 Dec 2015, Jan Beulich wrote: >> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote: >> > On Tue, 24 Nov 2015, Jan Beulich wrote: >> >> Now that the hypervisor intercepts all config space writes and monitors >> >> changes to the masking flags, this undoes the main effect of the >> >> XSA-129 fix, exposing the masking capability again to guests. > > Could you please mention the relevant commit ids in Xen? It's just one (which I've now added a reference to), unless you want all the prereqs listed. > What happens if QEMU, with this change, is running on top of an older > Xen that doesn't intercepts all config space writes? The security issue would resurface. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- >> >> TBD: We probably need to deal with running on an older hypervisor. I >> >> can't, however, immediately see a way for qemu to find out. >> > >> > Actually QEMU has already an infrastructure to detect the hypervisor >> > version at compile time, see include/hw/xen/xen_common.h. You could >> > #define the right emu_mask depending on the hypervisor. >> >> We don't want compile time detection here, but runtime one. > > I guess the issue is that a fix was backported to Xen that changed its > behaviour in past releases, right? No, we shouldn't try to guess whether this is present in any pre-4.6 hypervisors; we should simply accept that maskable MSI is not available for guests there, because ... > Is there a way to detect the presence of this fix in Xen, by invoking an > hypercall and checking the returned values and error numbers? ... there's nothing to (reliably) probe here. This really is just an implementation detail of the hypervisor, and hence a version check is all we have available. Jan
On Mon, 7 Dec 2015, Jan Beulich wrote: > >>> On 07.12.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote: > > On Mon, 7 Dec 2015, Jan Beulich wrote: > >> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote: > >> > On Tue, 24 Nov 2015, Jan Beulich wrote: > >> >> Now that the hypervisor intercepts all config space writes and monitors > >> >> changes to the masking flags, this undoes the main effect of the > >> >> XSA-129 fix, exposing the masking capability again to guests. > > > > Could you please mention the relevant commit ids in Xen? > > It's just one (which I've now added a reference to), unless you want > all the prereqs listed. One is probably OK. > > What happens if QEMU, with this change, is running on top of an older > > Xen that doesn't intercepts all config space writes? > > The security issue would resurface. > > >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >> >> --- > >> >> TBD: We probably need to deal with running on an older hypervisor. I > >> >> can't, however, immediately see a way for qemu to find out. > >> > > >> > Actually QEMU has already an infrastructure to detect the hypervisor > >> > version at compile time, see include/hw/xen/xen_common.h. You could > >> > #define the right emu_mask depending on the hypervisor. > >> > >> We don't want compile time detection here, but runtime one. > > > > I guess the issue is that a fix was backported to Xen that changed its > > behaviour in past releases, right? > > No, we shouldn't try to guess whether this is present in any pre-4.6 > hypervisors; we should simply accept that maskable MSI is not > available for guests there, because ... > > > Is there a way to detect the presence of this fix in Xen, by invoking an > > hypercall and checking the returned values and error numbers? > > ... there's nothing to (reliably) probe here. This really is just an > implementation detail of the hypervisor, and hence a version check > is all we have available. In that case, I think we should stay on the safe side, and only expose the masking capability (only take into effects the changes that this patch makes) for Xen >= 4.7. What do you think?
>>> On 11.12.15 at 15:33, <stefano.stabellini@eu.citrix.com> wrote: > On Mon, 7 Dec 2015, Jan Beulich wrote: >> >>> On 07.12.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote: >> > On Mon, 7 Dec 2015, Jan Beulich wrote: >> >> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote: >> >> > On Tue, 24 Nov 2015, Jan Beulich wrote: >> >> >> TBD: We probably need to deal with running on an older hypervisor. I >> >> >> can't, however, immediately see a way for qemu to find out. >> >> > >> >> > Actually QEMU has already an infrastructure to detect the hypervisor >> >> > version at compile time, see include/hw/xen/xen_common.h. You could >> >> > #define the right emu_mask depending on the hypervisor. >> >> >> >> We don't want compile time detection here, but runtime one. >> > >> > I guess the issue is that a fix was backported to Xen that changed its >> > behaviour in past releases, right? >> >> No, we shouldn't try to guess whether this is present in any pre-4.6 >> hypervisors; we should simply accept that maskable MSI is not >> available for guests there, because ... >> >> > Is there a way to detect the presence of this fix in Xen, by invoking an >> > hypercall and checking the returned values and error numbers? >> >> ... there's nothing to (reliably) probe here. This really is just an >> implementation detail of the hypervisor, and hence a version check >> is all we have available. > > In that case, I think we should stay on the safe side, and only expose > the masking capability (only take into effects the changes that this > patch makes) for Xen >= 4.7. > > What do you think? That's what I suggested, with the hope of getting a hint where the necessary infrastructure is (somehow I didn't find any run time version dependent code to clone from). Jan
On Fri, 11 Dec 2015, Jan Beulich wrote: > >>> On 11.12.15 at 15:33, <stefano.stabellini@eu.citrix.com> wrote: > > On Mon, 7 Dec 2015, Jan Beulich wrote: > >> >>> On 07.12.15 at 15:56, <stefano.stabellini@eu.citrix.com> wrote: > >> > On Mon, 7 Dec 2015, Jan Beulich wrote: > >> >> >>> On 07.12.15 at 13:45, <stefano.stabellini@eu.citrix.com> wrote: > >> >> > On Tue, 24 Nov 2015, Jan Beulich wrote: > >> >> >> TBD: We probably need to deal with running on an older hypervisor. I > >> >> >> can't, however, immediately see a way for qemu to find out. > >> >> > > >> >> > Actually QEMU has already an infrastructure to detect the hypervisor > >> >> > version at compile time, see include/hw/xen/xen_common.h. You could > >> >> > #define the right emu_mask depending on the hypervisor. > >> >> > >> >> We don't want compile time detection here, but runtime one. > >> > > >> > I guess the issue is that a fix was backported to Xen that changed its > >> > behaviour in past releases, right? > >> > >> No, we shouldn't try to guess whether this is present in any pre-4.6 > >> hypervisors; we should simply accept that maskable MSI is not > >> available for guests there, because ... > >> > >> > Is there a way to detect the presence of this fix in Xen, by invoking an > >> > hypercall and checking the returned values and error numbers? > >> > >> ... there's nothing to (reliably) probe here. This really is just an > >> implementation detail of the hypervisor, and hence a version check > >> is all we have available. > > > > In that case, I think we should stay on the safe side, and only expose > > the masking capability (only take into effects the changes that this > > patch makes) for Xen >= 4.7. > > > > What do you think? > > That's what I suggested, with the hope of getting a hint where the > necessary infrastructure is (somehow I didn't find any run time > version dependent code to clone from). It is not possible to do this at runtime. I think we should do this at compile time because in any case it is not supported to run a QEMU built for a given Xen version on a different Xen version. The infrastructure to do this at compile time is in ./include/hw/xen/xen_common.h
On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote: > > It is not possible to do this at runtime. I think we should do this at > compile time because in any case it is not supported to run a QEMU built > for a given Xen version on a different Xen version. I am currently working pretty hard to make this possible in the future, it would be a shame to add another reason it wasn't possible at this stage. I proposed (in <1445442435.9563.184.camel@citrix.com>) that as well as the various stable libraries extracted from libxenctrl we will probably also want to have a libxendevicemodel.so at some point, to provide a stable way to interface with all the stuff which being a DM involves. Maybe that library could contain a way to get this information? (In which case it could be hardcoded at compile time now and I'll see what I can do when I get to producing the library). For the original issue here, could the flag be exposed as a XEN_SYSCTL_PHYSCAP_???? Ian.
>>> On 11.12.15 at 17:56, <ian.campbell@citrix.com> wrote: > On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote: >> >> It is not possible to do this at runtime. I think we should do this at >> compile time because in any case it is not supported to run a QEMU built >> for a given Xen version on a different Xen version. > > I am currently working pretty hard to make this possible in the future, it > would be a shame to add another reason it wasn't possible at this stage. And I don't think it's not possible - if anything, the infrastructure to do so is just missing. I'm definitely not going to make this a build time check, since I deem it very wrong namely when considering --with-system-qemu (as in that case there shouldn't be any dependency on the precise Xen tools versions in use - using plural intentionally here to point out the possibility of multiple ones being present). Jan
On Fri, 11 Dec 2015, Ian Campbell wrote: > On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote: > > > > It is not possible to do this at runtime. I think we should do this at > > compile time because in any case it is not supported to run a QEMU built > > for a given Xen version on a different Xen version. > > I am currently working pretty hard to make this possible in the future, it > would be a shame to add another reason it wasn't possible at this stage. > > I proposed (in <1445442435.9563.184.camel@citrix.com>) that as well as the > various stable libraries extracted from libxenctrl we will probably also > want to have a libxendevicemodel.so at some point, to provide a stable way > to interface with all the stuff which being a DM involves. I understand the direction we are heading toward, but unfortunately we are still pretty far from it. I don't think we want to block this patch until we have a stable libxendevicemodel ABI? Also this particular change regards PCI passthrough, which is not convered by the proposed ABI yet. > Maybe that library could contain a way to get this information? (In which > case it could be hardcoded at compile time now and I'll see what I can do > when I get to producing the library). Given the choice, I would rather have only compile time or only run time Xen version checks in QEMU and not both to avoid complexity. Especially as long as the underlying libraries don't make any stability guarantees. > For the original issue here, could the flag be exposed as a > XEN_SYSCTL_PHYSCAP_???? Feature flags are welcome and the best course of action in my opinion.
On Mon, 14 Dec 2015, Jan Beulich wrote: > >>> On 11.12.15 at 17:56, <ian.campbell@citrix.com> wrote: > > On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote: > >> > >> It is not possible to do this at runtime. I think we should do this at > >> compile time because in any case it is not supported to run a QEMU built > >> for a given Xen version on a different Xen version. > > > > I am currently working pretty hard to make this possible in the future, it > > would be a shame to add another reason it wasn't possible at this stage. > > And I don't think it's not possible - if anything, the infrastructure to > do so is just missing. I'm definitely not going to make this a build time > check, since I deem it very wrong namely when considering > --with-system-qemu (as in that case there shouldn't be any > dependency on the precise Xen tools versions in use - using plural > intentionally here to point out the possibility of multiple ones being > present). Compile time checks are indeed suboptimal, but so are runtime checks: what if we backport the fix to more Xen releases? What if we revert the fix on the Xen tree for any reason? I think that a feature flag is the best course of action.
On Mon, 2015-12-14 at 11:19 +0000, Stefano Stabellini wrote: > On Fri, 11 Dec 2015, Ian Campbell wrote: > > On Fri, 2015-12-11 at 16:44 +0000, Stefano Stabellini wrote: > > > > > > It is not possible to do this at runtime. I think we should do this > > > at > > > compile time because in any case it is not supported to run a QEMU > > > built > > > for a given Xen version on a different Xen version. > > > > I am currently working pretty hard to make this possible in the future, > > it > > would be a shame to add another reason it wasn't possible at this > > stage. > > > > I proposed (in <1445442435.9563.184.camel@citrix.com>) that as well as > > the > > various stable libraries extracted from libxenctrl we will probably > > also > > want to have a libxendevicemodel.so at some point, to provide a stable > > way > > to interface with all the stuff which being a DM involves. > > I understand the direction we are heading toward, but unfortunately we > are still pretty far from it. I don't think we want to block this patch > until we have a stable libxendevicemodel ABI? No, but I would appreciate if such things were explicitly considered on a case by case by case basis rather than just bundled under a generic "it's not possible yet", since there may be cases where we want to hold off, or more likely where doing something a particular way now will ease things for the transition in the future. > Also this particular > change regards PCI passthrough, which is not convered by the proposed > ABI yet. > > > > Maybe that library could contain a way to get this information? (In > > which > > case it could be hardcoded at compile time now and I'll see what I can > > do > > when I get to producing the library). > > Given the choice, I would rather have only compile time or only run time > Xen version checks in QEMU and not both to avoid complexity. Especially > as long as the underlying libraries don't make any stability guarantees. "that library" obviously will make such guarantees as a matter of design. Ian.
>>> On 11.12.15 at 17:56, <ian.campbell@citrix.com> wrote: > For the original issue here, could the flag be exposed as a > XEN_SYSCTL_PHYSCAP_???? Yes, I think it could, albeit calling this a "capability" or "feature" seems odd (since really the original behavior was bogus/buggy). But - with sysctl not being a stable interface, is making qemu use this actually a good idea? I.e. won't we paint ourselves into the corner of needing to write compatibility wrappers in qemu whenever XEN_SYSCTL_physinfo (or its libxc wrapper) changes? Jan
On Mon, 14 Dec 2015, Jan Beulich wrote: > >>> On 11.12.15 at 17:56, <ian.campbell@citrix.com> wrote: > > For the original issue here, could the flag be exposed as a > > XEN_SYSCTL_PHYSCAP_???? > > Yes, I think it could, albeit calling this a "capability" or "feature" > seems odd (since really the original behavior was bogus/buggy). > But - with sysctl not being a stable interface, is making qemu use > this actually a good idea? I.e. won't we paint ourselves into the > corner of needing to write compatibility wrappers in qemu > whenever XEN_SYSCTL_physinfo (or its libxc wrapper) changes? Even though it might increase the number of compatibility wrappers that we need today, I think that Ian will be able to eventually switch it over to a stable interface?
--- a/hw/xen/xen_pt_config_init.c +++ b/hw/xen/xen_pt_config_init.c @@ -1333,7 +1333,7 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] .init_val = 0x0000, .res_mask = 0xFE00, .ro_mask = 0x018E, - .emu_mask = 0x017E, + .emu_mask = 0x007E, .init = xen_pt_msgctrl_reg_init, .u.w.read = xen_pt_word_reg_read, .u.w.write = xen_pt_msgctrl_reg_write, @@ -1387,8 +1387,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] .offset = PCI_MSI_MASK_32, .size = 4, .init_val = 0x00000000, - .ro_mask = 0xFFFFFFFF, - .emu_mask = 0xFFFFFFFF, + .ro_mask = 0x00000000, + .emu_mask = 0x00000000, .init = xen_pt_mask_reg_init, .u.dw.read = xen_pt_long_reg_read, .u.dw.write = xen_pt_long_reg_write, @@ -1398,8 +1398,8 @@ static XenPTRegInfo xen_pt_emu_reg_msi[] .offset = PCI_MSI_MASK_64, .size = 4, .init_val = 0x00000000, - .ro_mask = 0xFFFFFFFF, - .emu_mask = 0xFFFFFFFF, + .ro_mask = 0x00000000, + .emu_mask = 0x00000000, .init = xen_pt_mask_reg_init, .u.dw.read = xen_pt_long_reg_read, .u.dw.write = xen_pt_long_reg_write,