linux-kernel - Re: [PATCH 2/2] vfio/type1: Give hardware MSI regions precedence

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <60052d02-276e-ed12-f372-14ce166f50ed@redhat.com>
Date:   Sun, 30 Jul 2017 12:39:37 +0200
From:   Auger Eric <eric.auger@...hat.com>
To:     Robin Murphy <robin.murphy@....com>, alex.williamson@...hat.com
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        shameerali.kolothum.thodi@...wei.com, marc.zyngier@....com
Subject: Re: [PATCH 2/2] vfio/type1: Give hardware MSI regions precedence

Hi Robin,
On 27/07/2017 16:54, Robin Murphy wrote:
> If the IOMMU driver advertises 'real' reserved regions for MSIs, but
> still includes the software-managed region as well, we are currently
> blind to the former and will configure the IOMMU domain to map MSIs into
> the latter, which is unlikely to work as expected.
> 
> Since it would take a ridiculous hardware topology for both regions to
> be valid (which would be rather difficult to support in general), we
> should be safe to assume that the presence of any hardware regions makes
> the software region irrelevant. However, the IOMMU driver might still
> advertise the software region by default, particularly if the hardware
> regions are filled in elsewhere by generic code, so it might not be fair
> for VFIO to be super-strict about not mixing them. To that end, make
> vfio_iommu_has_sw_msi() robust against the presence of both region types
> at once, so that we end up doing what is almost certainly right, rather
> than what is almost certainly wrong.
> 
> Signed-off-by: Robin Murphy <robin.murphy@....com>
Reviewed-by: Eric Auger <eric.auger@...hat.com>

Thanks

Eric
> ---
>  drivers/vfio/vfio_iommu_type1.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 2328be628f21..92155cce926d 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1169,13 +1169,21 @@ static bool vfio_iommu_has_sw_msi(struct iommu_group *group, phys_addr_t *base)
>  	INIT_LIST_HEAD(&group_resv_regions);
>  	iommu_get_group_resv_regions(group, &group_resv_regions);
>  	list_for_each_entry(region, &group_resv_regions, list) {
> +		/*
> +		 * The presence of any 'real' MSI regions should take
> +		 * precedence over the software-managed one if the
> +		 * IOMMU driver happens to advertise both types.
> +		 */
> +		if (region->type == IOMMU_RESV_MSI) {
> +			ret = false;
> +			break;
> +		}
> +
>  		if (region->type == IOMMU_RESV_SW_MSI) {
>  			*base = region->start;
>  			ret = true;
> -			goto out;
>  		}
>  	}
> -out:
>  	list_for_each_entry_safe(region, next, &group_resv_regions, list)
>  		kfree(region);
>  	return ret;
>