Re: [PATCH 3.10 037/319] xen/pciback: Save xen_pci_op commands before processing it

[Konrad Rzeszutek Wilk]

On Sun, Feb 05, 2017 at 08:20:24PM +0100, Willy Tarreau wrote:
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> 
> commit 8135cf8b092723dbfcc611fe6fdcb3a36c9951c5 upstream.

You also need:

commit d159457b84395927b5a52adb72f748dd089ad5e5
Author: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date:   Thu Feb 11 16:10:24 2016 -0500

    xen/pciback: Save the number of MSI-X entries to be copied later.

    Commit 8135cf8b092723dbfcc611fe6fdcb3a36c9951c5 (xen/pciback: Save
    xen_pci_op commands before processing it) broke enabling MSI-X because
    it would never copy the resulting vectors into the response.  The
    number of vectors requested was being overwritten by the return value
    (typically zero for success).


> 
> Double fetch vulnerabilities that happen when a variable is
> fetched twice from shared memory but a security check is only
> performed the first time.
> 
> The xen_pcibk_do_op function performs a switch statements on the op->cmd
> value which is stored in shared memory. Interestingly this can result
> in a double fetch vulnerability depending on the performed compiler
> optimization.
> 
> This patch fixes it by saving the xen_pci_op command before
> processing it. We also use 'barrier' to make sure that the
> compiler does not perform any optimization.
> 
> This is part of XSA155.
> 
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Signed-off-by: Jan Beulich <JBeulich@xxxxxxxx>
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Cc: "Jan Beulich" <JBeulich@xxxxxxxx>
> Signed-off-by: Willy Tarreau <w@xxxxxx>
> ---
>  drivers/xen/xen-pciback/pciback.h     |  1 +
>  drivers/xen/xen-pciback/pciback_ops.c | 15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/xen-pciback/pciback.h b/drivers/xen/xen-pciback/pciback.h
> index f72af87..560b3ec 100644
> --- a/drivers/xen/xen-pciback/pciback.h
> +++ b/drivers/xen/xen-pciback/pciback.h
> @@ -37,6 +37,7 @@ struct xen_pcibk_device {
>  	struct xen_pci_sharedinfo *sh_info;
>  	unsigned long flags;
>  	struct work_struct op_work;
> +	struct xen_pci_op op;
>  };
>  
>  struct xen_pcibk_dev_data {
> diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c
> index b98cf0c..32f83f0 100644
> --- a/drivers/xen/xen-pciback/pciback_ops.c
> +++ b/drivers/xen/xen-pciback/pciback_ops.c
> @@ -295,9 +295,11 @@ void xen_pcibk_do_op(struct work_struct *data)
>  		container_of(data, struct xen_pcibk_device, op_work);
>  	struct pci_dev *dev;
>  	struct xen_pcibk_dev_data *dev_data = NULL;
> -	struct xen_pci_op *op = &pdev->sh_info->op;
> +	struct xen_pci_op *op = &pdev->op;
>  	int test_intx = 0;
>  
> +	*op = pdev->sh_info->op;
> +	barrier();
>  	dev = xen_pcibk_get_pci_dev(pdev, op->domain, op->bus, op->devfn);
>  
>  	if (dev == NULL)
> @@ -339,6 +341,17 @@ void xen_pcibk_do_op(struct work_struct *data)
>  		if ((dev_data->enable_intx != test_intx))
>  			xen_pcibk_control_isr(dev, 0 /* no reset */);
>  	}
> +	pdev->sh_info->op.err = op->err;
> +	pdev->sh_info->op.value = op->value;
> +#ifdef CONFIG_PCI_MSI
> +	if (op->cmd == XEN_PCI_OP_enable_msix && op->err == 0) {
> +		unsigned int i;
> +
> +		for (i = 0; i < op->value; i++)
> +			pdev->sh_info->op.msix_entries[i].vector =
> +				op->msix_entries[i].vector;
> +	}
> +#endif
>  	/* Tell the driver domain that we're done. */
>  	wmb();
>  	clear_bit(_XEN_PCIF_active, (unsigned long *)&pdev->sh_info->flags);
> -- 
> 2.8.0.rc2.1.gbe9624a
>