Re: [PATCH v4 4/6] vfio/type1: check dma map request is within a valid iova range

[Auger Eric]

Hi Shameer,

On 27/02/18 10:57, Shameerali Kolothum Thodi wrote:
> 
> 
>> -----Original Message-----
>> From: Auger Eric [mailto:eric.auger@xxxxxxxxxx]
>> Sent: Tuesday, February 27, 2018 8:27 AM
>> To: Alex Williamson <alex.williamson@xxxxxxxxxx>
>> Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
>> pmorel@xxxxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; John Garry
>> <john.garry@xxxxxxxxxx>; xuwei (O) <xuwei5@xxxxxxxxxx>; Robin Murphy
>> <robin.murphy@xxxxxxx>
>> Subject: Re: [PATCH v4 4/6] vfio/type1: check dma map request is within a valid
>> iova range
>>
>> Hi,
>> On 27/02/18 00:13, Alex Williamson wrote:
>>> On Mon, 26 Feb 2018 23:05:43 +0100
>>> Auger Eric <eric.auger@xxxxxxxxxx> wrote:
>>>
>>>> Hi Shameer,
>>>>
>>>> [Adding Robin in CC]
>>>> On 21/02/18 13:22, Shameer Kolothum wrote:
>>>>> This checks and rejects any dma map request outside valid iova
>>>>> range.
>>>>>
>>>>> Signed-off-by: Shameer Kolothum
>> <shameerali.kolothum.thodi@xxxxxxxxxx>
>>>>> ---
>>>>>  drivers/vfio/vfio_iommu_type1.c | 22 ++++++++++++++++++++++
>>>>>  1 file changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
>> b/drivers/vfio/vfio_iommu_type1.c
>>>>> index a80884e..3049393 100644
>>>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>>>> @@ -970,6 +970,23 @@ static int vfio_pin_map_dma(struct vfio_iommu
>> *iommu, struct vfio_dma *dma,
>>>>>  	return ret;
>>>>>  }
>>>>>
>>>>> +/*
>>>>> + * Check dma map request is within a valid iova range
>>>>> + */
>>>>> +static bool vfio_iommu_iova_dma_valid(struct vfio_iommu *iommu,
>>>>> +				dma_addr_t start, dma_addr_t end)
>>>>> +{
>>>>> +	struct list_head *iova = &iommu->iova_list;
>>>>> +	struct vfio_iova *node;
>>>>> +
>>>>> +	list_for_each_entry(node, iova, list) {
>>>>> +		if ((start >= node->start) && (end <= node->end))
>>>>> +			return true;
>>>> I am now confused by the fact this change will prevent existing QEMU
>>>> from working with this series on some platforms. For instance QEMU virt
>>>> machine GPA space collides with Seattle PCI host bridge windows. On ARM
>>>> the smmu and smmuv3 drivers report the PCI host bridge windows as
>>>> reserved regions which does not seem to be the case on other platforms.
>>>> The change happened in commit
>> 273df9635385b2156851c7ee49f40658d7bcb29d
>>>> (iommu/dma: Make PCI window reservation generic).
>>>>
>>>> For background, we already discussed the topic after LPC 2016. See
>>>> https://www.spinics.net/lists/kernel/msg2379607.html.
>>>>
>>>> So is it the right choice to expose PCI host bridge windows as reserved
>>>> regions? If yes shouldn't we make a difference between those and MSI
>>>> windows in this series and do not reject any user space DMA_MAP attempt
>>>> within PCI host bridge windows.
>>>
>>> If the QEMU machine GPA collides with a reserved region today, then
>>> either:
>>>
>>> a) The mapping through the IOMMU works and the reserved region is wrong
>>>
>>> or
>>>
>>> b) The mapping doesn't actually work, QEMU is at risk of data loss by
>>> being told that it worked, and we're justified in changing that
>>> behavior.
>>>
>>> Without knowing the specifics of SMMU, it doesn't particularly make
>>> sense to me to mark the entire PCI hierarchy MMIO range as reserved,
>>> unless perhaps the IOMMU is incapable of translating those IOVAs.
>> to me the limitation does not come from the smmu itself, which is a
>> separate HW block sitting between the root complex and the interconnect.
>> If ACS is not enforced by the PCIe subsystem, the transaction will never
>> reach the IOMMU.
> 
> True. And we do have one such platform where ACS is not enforced but 
> reserving the regions and possibly creating holes while launching VM will
> make it secure. But I do wonder how we will solve the device grouping
> in such cases. 
> 
> The Seattle PCI host bridge windows case you mentioned has any pci quirk 
> to claim that they support ACS?
No there is none to my knowledge. I am applying Alex' not upstream ACS
overwrite patch.

Thanks

Eric
>  
>> In the case of such overlap, shouldn't we just warn the end-user that
>> this situation is dangerous instead of forbidding the use case which
>> worked "in most cases" until now.
> 
> Yes, may be something similar to the allow_unsafe_interrupts case, if
> that is acceptable.
> 
> Thanks,
> Shameer
>  
>>> Are we trying to prevent untranslated p2p with this reserved range?
>>> That's not necessarily a terrible idea, but it seems that doing it for
>>> that purpose would need to be a lot smarter, taking into account ACS
>>> and precisely selecting ranges within the peer address space that would
>>> be untranslated.  Perhaps only populated MMIO within non-ACS
>>> hierarchies.  Thanks,
>>
>> Indeed taking into account the ACS capability would refine the
>> situations where a risk exists.
>>
>> Thanks
>>
>> Eric
>>>
>>> Alex
>>>