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

[Michael S. Tsirkin]

On Tue, Mar 31, 2015 at 03:18:03PM +0100, Stefano Stabellini wrote:
> From: Jan Beulich <address@hidden>
> 
> 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 <address@hidden>
> Reviewed-by: Stefano Stabellini <address@hidden>
> Acked-by: Ian Campbell <address@hidden>

The patch description seems somewhat incorrect to me.
UR should not be fatal to the system, and it's not platform
specific.

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.

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.


> 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.


>      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 */