Re: [Qemu-devel] [PATCH][XSA-126] xen: limit guest control of PCI command register

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Apr 01, 2015 at 10:20:06AM +0100, Stefano Stabellini wrote:
> CC'ing the author of the patch and xen-devel.
> FYI I think that Jan is going to be on vacation for a couple of weeks.
> 
> On Wed, 1 Apr 2015, Michael S. Tsirkin wrote:
> > On Tue, Mar 31, 2015 at 03:18:03PM +0100, Stefano Stabellini wrote:
> > > From: Jan Beulich <jbeulich@suse.com>
> > > 
> > > Otherwise the guest can abuse that control to cause e.g. PCIe
> > > Unsupported Request responses (by disabling memory and/or I/O decoding
> > > and subsequently causing [CPU side] accesses to the respective address
> > > ranges), which (depending on system configuration) may be fatal to the
> > > host.
> > > 
> > > This is CVE-2015-2756 / XSA-126.
> > > 
> > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > Reviewed-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > The patch description seems somewhat incorrect to me.
> > UR should not be fatal to the system, and it's not platform
> > specific.
> 
> I think that people have been able to repro this, but I'll let Jan
> comment on it.
> 
> 
> > In particular, there could be more reasons for devices
> > to generate URs, for example, if they get a transaction
> > during FLR. I don't think we ever tried to prevent this.
> 
> That cannot be triggered by guest behaviour.

Emulated bus reset often triggers FLR, but I can't speak for
Xen here.  Many devices have vendor specific reset registers though.
I don't believe anyone has a comprehensive list of such devices.

Fundamendally IOMMU is hosts's only protection. IOMMU doesn't
protect against URs so they mustn't trigger system errors.


> 
> > Here's the description from pci express spec:
> > 
> > IMPLEMENTATION NOTE
> > Software UR Reporting Compatibility with 1.0a Devices
> > 
> > 	With 1.0a device Functions, 96 if the Unsupported Request Reporting Enable bit is set, the Function
> > 	when operating as a Completer will send an uncorrectable error Message (if enabled) when a UR
> > 	error is detected. On platforms where an uncorrectable error Message is handled as a System Error,
> > 	this will break PC-compatible Configuration Space probing, so software/firmware on such
> > 	platforms may need to avoid setting the Unsupported Request Reporting Enable bit.
> > 	With device Functions implementing Role-Based Error Reporting, setting the Unsupported Request
> > 	Reporting Enable bit will not interfere with PC-compatible Configuration Space probing, assuming
> > 	that the severity for UR is left at its default of non-fatal. However, setting the Unsupported Request
> > 	Reporting Enable bit will enable the Function to report UR errors detected with posted Requests,
> > 	helping avoid this case for potential silent data corruption.
> > 	On platforms where robust error handling and PC-compatible Configuration Space probing is
> > 	required, it is suggested that software or firmware have the Unsupported Request Reporting Enable
> > 	bit Set for Role-Based Error Reporting Functions, but clear for 1.0a Functions. Software or
> > 	firmware can distinguish the two classes of Functions by examining the Role-Based Error Reporting
> > 	bit in the Device Capabilities register.
> > 
> > 
> > What I think you have is a very old 1.0a system, and host set Unsupported
> > Request Reporting Enable.
> > 
> > The right thing is for host kernel to do is what the note says, and disable UR
> > reporting for this system.
> > 
> > As a work around for broken kernels, this is OK I guess, though
> > guest and host being out of sync is always a risk.
> > But I think it's a good idea to add documentation explaining
> > this, and work on the correct fix in linux, before we
> > add workarounds.
> 
> There can be guest kernels other than Linux, we cannot fix them all, and
> we cannot allow PCI passthrough only with Linux guests.

Are you trying to fix guest crashes, or host crashes?
If guest crashes, how is this a CVE?

What I said above has nothing to do with guest bugs.
I think the bug is in whoever configured the host pci bridge.
That's host kernel, not guest kernel.


> 
> > > diff --git a/hw/xen/xen_pt.c b/hw/xen/xen_pt.c
> > > index f2893b2..d095c08 100644
> > > --- a/hw/xen/xen_pt.c
> > > +++ b/hw/xen/xen_pt.c
> > > @@ -388,7 +388,7 @@ static const MemoryRegionOps ops = {
> > >      .write = xen_pt_bar_write,
> > >  };
> > >  
> > > -static int xen_pt_register_regions(XenPCIPassthroughState *s)
> > > +static int xen_pt_register_regions(XenPCIPassthroughState *s, uint16_t *cmd)
> > >  {
> > >      int i = 0;
> > >      XenHostPCIDevice *d = &s->real_device;
> > > @@ -406,6 +406,7 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s)
> > >  
> > >          if (r->type & XEN_HOST_PCI_REGION_TYPE_IO) {
> > >              type = PCI_BASE_ADDRESS_SPACE_IO;
> > > +            *cmd |= PCI_COMMAND_IO;
> > >          } else {
> > >              type = PCI_BASE_ADDRESS_SPACE_MEMORY;
> > >              if (r->type & XEN_HOST_PCI_REGION_TYPE_PREFETCH) {
> > > @@ -414,6 +415,7 @@ static int xen_pt_register_regions(XenPCIPassthroughState *s)
> > >              if (r->type & XEN_HOST_PCI_REGION_TYPE_MEM_64) {
> > >                  type |= PCI_BASE_ADDRESS_MEM_TYPE_64;
> > >              }
> > > +            *cmd |= PCI_COMMAND_MEMORY;
> > >          }
> > >  
> > >          memory_region_init_io(&s->bar[i], OBJECT(s), &ops, &s->dev,
> > > @@ -638,6 +640,7 @@ static int xen_pt_initfn(PCIDevice *d)
> > >      XenPCIPassthroughState *s = DO_UPCAST(XenPCIPassthroughState, dev, d);
> > >      int rc = 0;
> > >      uint8_t machine_irq = 0;
> > > +    uint16_t cmd = 0;
> > >      int pirq = XEN_PT_UNASSIGNED_PIRQ;
> > >  
> > >      /* register real device */
> > > @@ -672,7 +675,7 @@ static int xen_pt_initfn(PCIDevice *d)
> > >      s->io_listener = xen_pt_io_listener;
> > >  
> > >      /* Handle real device's MMIO/PIO BARs */
> > > -    xen_pt_register_regions(s);
> > > +    xen_pt_register_regions(s, &cmd);
> > >  
> > >      /* reinitialize each config register to be emulated */
> > >      if (xen_pt_config_init(s)) {
> > > @@ -736,6 +739,11 @@ static int xen_pt_initfn(PCIDevice *d)
> > >      }
> > >  
> > >  out:
> > > +    if (cmd) {
> > > +        xen_host_pci_set_word(&s->real_device, PCI_COMMAND,
> > > +                              pci_get_word(d->config + PCI_COMMAND) | cmd);
> > > +    }
> > > +
> > 
> > Is this writing to host device, forcing cmd and io bits to enabled simply
> > because they are present? If yes, I think that's wrong: you don't know whether
> > bios enabled them, if it didn't host BARs might conflict with other devices,
> > and this will crash host.  I don't see why you are touching host command
> > register at all.  The point in the description is to avoid touching host.
> 
> pciback clears the command register when seizing the device:
> 
> pcistub_seize() -> pcistub_init_device() -> xen_pcibk_reset_device()


I think you have a bug then.  You must keep memory/io enable bits that
bios set for you, otherwise you risk conflicts with other devices,
or conflicts between BARs. I guess you do this for BAR values,
same applies here.




> 
> > >      memory_listener_register(&s->memory_listener, &s->dev.bus_master_as);
> > >      memory_listener_register(&s->io_listener, &address_space_io);
> > >      XEN_PT_LOG(d,
> > > diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> > > index d99c22e..95a51db 100644
> > > --- a/hw/xen/xen_pt_config_init.c
> > > +++ b/hw/xen/xen_pt_config_init.c
> > > @@ -286,23 +286,6 @@ static int xen_pt_irqpin_reg_init(XenPCIPassthroughState *s,
> > >  }
> > >  
> > >  /* Command register */
> > > -static int xen_pt_cmd_reg_read(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> > > -                               uint16_t *value, uint16_t valid_mask)
> > > -{
> > > -    XenPTRegInfo *reg = cfg_entry->reg;
> > > -    uint16_t valid_emu_mask = 0;
> > > -    uint16_t emu_mask = reg->emu_mask;
> > > -
> > > -    if (s->is_virtfn) {
> > > -        emu_mask |= PCI_COMMAND_MEMORY;
> > > -    }
> > > -
> > > -    /* emulate word register */
> > > -    valid_emu_mask = emu_mask & valid_mask;
> > > -    *value = XEN_PT_MERGE_VALUE(*value, cfg_entry->data, ~valid_emu_mask);
> > > -
> > > -    return 0;
> > > -}
> > >  static int xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> > >                                  uint16_t *val, uint16_t dev_value,
> > >                                  uint16_t valid_mask)
> > > @@ -310,18 +293,13 @@ static int xen_pt_cmd_reg_write(XenPCIPassthroughState *s, XenPTReg *cfg_entry,
> > >      XenPTRegInfo *reg = cfg_entry->reg;
> > >      uint16_t writable_mask = 0;
> > >      uint16_t throughable_mask = 0;
> > > -    uint16_t emu_mask = reg->emu_mask;
> > > -
> > > -    if (s->is_virtfn) {
> > > -        emu_mask |= PCI_COMMAND_MEMORY;
> > > -    }
> > >  
> > >      /* modify emulate register */
> > >      writable_mask = ~reg->ro_mask & valid_mask;
> > >      cfg_entry->data = XEN_PT_MERGE_VALUE(*val, cfg_entry->data, writable_mask);
> > >  
> > >      /* create value for writing to I/O device register */
> > > -    throughable_mask = ~emu_mask & valid_mask;
> > > +    throughable_mask = ~reg->emu_mask & valid_mask;
> > >  
> > >      if (*val & PCI_COMMAND_INTX_DISABLE) {
> > >          throughable_mask |= PCI_COMMAND_INTX_DISABLE;
> > > @@ -603,9 +581,9 @@ static XenPTRegInfo xen_pt_emu_reg_header0[] = {
> > >          .size       = 2,
> > >          .init_val   = 0x0000,
> > >          .ro_mask    = 0xF880,
> > > -        .emu_mask   = 0x0740,
> > > +        .emu_mask   = 0x0743,
> > >          .init       = xen_pt_common_reg_init,
> > > -        .u.w.read   = xen_pt_cmd_reg_read,
> > > +        .u.w.read   = xen_pt_word_reg_read,
> > >          .u.w.write  = xen_pt_cmd_reg_write,
> > >      },
> > >      /* Capabilities Pointer reg */
> >