Re: [PATCH v3 02/10] iommu/sva: Bind process address spaces to devices

[Jean-Philippe Brucker <jean-philippe.brucker@arm.com>]

On 26/09/2018 19:01, Jacob Pan wrote:
> On Mon, 24 Sep 2018 13:07:47 +0100
> Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:
> 
>> On 23/09/2018 04:05, Lu Baolu wrote:
>> > Hi,
>> > 
>> > On 09/21/2018 01:00 AM, Jean-Philippe Brucker wrote:  
>> >> Add bind() and unbind() operations to the IOMMU API. Bind()
>> >> returns a PASID that drivers can program in hardware, to let their
>> >> devices access an mm. This patch only adds skeletons for the
>> >> device driver API, most of the implementation is still missing.  
>> > 
>> > Is it possible that a malicious process can unbind a pasid which is
>> > used by another normal process?  
>> 
>> Yes, it's up to the device driver that calls unbind() to check that
>> the caller is allowed to unbind this PASID. We can't do it ourselves
>> since unbind() could also be called from a kernel thread for example
>> from a cleanup function in some workqueue, outside the context of the
>> process to unbind.

Actually I'm not too concerned about a process unbinding another one,
since in general only the kernel will hold the PASID values. Userspace
shouldn't even need to see them, so issuing unbind() with the wrong
PASID isn't an easy mistake.

> I am wondering if we can avoid the complexity around permission
> checking by simply _only_ allow bind/unbind() on current mm? what would
> be the missing use cases if we bind current only?
> It can also avoid other race such as unbind and mmu_notifier release
> call.

That's tempting but may be too restrictive. I just tried to copy what
the current AMD and Intel drivers do in their SVA implementation, but I
don't know if users will need all of it. At the moment the amdkfd driver
does unbind() from a workqueue, although moving to the generic API might
simplify things there.

Callers can easily enforce that only current->mm is passed to bind(). I
don't know if allowing a process to bind another one is a real use-case,
but the permission check on the device driver side is fairly easy, and
disallowing it wouldn't simplify iommu-sva.

Even if we allow bind() only on current, forcing unbind() to be done on
current means that the driver can't clean things up from a workqueue.
But you're right that this restriction would make things *much* simpler
for the exit()/unbind() race.

Thanks,
Jean