Re: [Xen-devel] [PATCH v4 5/6] xen/x86: add PHYSDEVOP_msi_set_enable

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Feb 07, 2019 at 01:07:48AM +0100, Marek Marczykowski-Górecki wrote:
> Allow device model running in stubdomain to enable/disable MSI(-X),
> bypassing pciback. While pciback is still used to access config space
> from within stubdomain, it refuse to write to
> PCI_MSI_FLAGS_ENABLE/PCI_MSIX_FLAGS_ENABLE in non-permissive mode. Which
> is the right thing to do for PV domain (the main use case for pciback),
> as PV domain should use XEN_PCI_OP_* commands for that. Unfortunately
> those commands are not good for stubdomain use, as they configure MSI in
> dom0's kernel too, which should not happen for HVM domain.
> 
> This new physdevop is allowed only for stubdomain controlling the domain
> which own the device.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
> Changes in v3:
>  - new patch
> Changes in v4:
>  - adjust code style
>  - s/msi_msix/msi/
>  - add msi_set_enable XSM hook
>  - flatten struct physdev_msi_set_enable
>  - add to include/xlat.lst
> 
> I'm not sure if XSM part is correct, compile-tested only, as I'm not
> sure how to set the policy.
> ---
>  xen/arch/x86/msi.c            | 24 ++++++++++++++++++++++++
>  xen/arch/x86/physdev.c        | 24 ++++++++++++++++++++++++
>  xen/arch/x86/x86_64/physdev.c |  4 ++++
>  xen/include/asm-x86/msi.h     |  1 +
>  xen/include/public/physdev.h  | 15 +++++++++++++++
>  xen/include/xlat.lst          |  1 +
>  xen/include/xsm/dummy.h       |  7 +++++++
>  xen/include/xsm/xsm.h         |  6 ++++++
>  xen/xsm/dummy.c               |  1 +
>  xen/xsm/flask/hooks.c         | 25 +++++++++++++++++++++++++
>  10 files changed, 108 insertions(+)
> 
> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> index babc414..c490c67 100644
> --- a/xen/arch/x86/msi.c
> +++ b/xen/arch/x86/msi.c
> @@ -1474,6 +1474,30 @@ int pci_restore_msi_state(struct pci_dev *pdev)
>      return 0;
>  }
>  
> +int msi_msix_set_enable(struct pci_dev *pdev, int mode, int enable)
> +{
> +    int ret;
> +
> +    ret = xsm_msi_set_enable(XSM_DM_PRIV, pdev->domain,
> +                             (pdev->seg << 16) | (pdev->bus << 8) | 
> pdev->devfn,

There's a helper for this: PCI_SBDF3.

> diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
> index 96d31aa..1c201fd 100644
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1084,6 +1084,30 @@ static int flask_pci_config_permission(struct domain 
> *d, uint32_t machine_bdf, u
>  
>  }
>  
> +static int flask_msi_set_enable(struct domain *d, uint32_t machine_bdf, 
> uint8_t mode, uint8_t enable)
> +{
> +    u32 dsid, rsid;
> +    int rc = -EPERM;
> +    struct avc_audit_data ad;
> +    u32 perm;

Nit: I think we are trying to get away from using u32, so I would use
uint32_t. I know the file makes heavy use of the short u32/64 types
but we should try to not introduce more.

> +
> +    AVC_AUDIT_DATA_INIT(&ad, DEV);
> +    ad.device = (unsigned long) machine_bdf;

Do you really need the cast here?

> +
> +    rc = security_device_sid(machine_bdf, &rsid);
> +    if ( rc )
> +        return rc;
> +
> +    perm = flask_iommu_resource_use_perm();
> +
> +    rc = avc_current_has_perm(rsid, SECCLASS_RESOURCE, perm, &ad);
> +    if ( rc )
> +        return rc;
> +
> +    dsid = domain_sid(d);
> +    return avc_current_has_perm(dsid, SECCLASS_RESOURCE, RESOURCE__SETUP, 
> &ad);

I think you need to add a line to xsm/flask/policy/access_vectors
noting that PHYSDEVOP_msi_set_enable makes use of the setup
permissions (like it's done for PHYSDEVOP_setup_gsi for example).

Anyway I'm not very familiar with XSM, so Daniel might have better
input on this.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel