linux-kernel - Re: [PATCH v4 20/22] iommu/vt-d: Add bind guest PASID support

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]

Date:   Fri, 5 Jul 2019 10:21:27 +0800
From:   Lu Baolu <baolu.lu@...ux.intel.com>
To:     Jacob Pan <jacob.jun.pan@...ux.intel.com>
Cc:     baolu.lu@...ux.intel.com, iommu@...ts.linux-foundation.org,
        LKML <linux-kernel@...r.kernel.org>,
        Joerg Roedel <joro@...tes.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Eric Auger <eric.auger@...hat.com>,
        Alex Williamson <alex.williamson@...hat.com>,
        Jean-Philippe Brucker <jean-philippe.brucker@....com>,
        Yi Liu <yi.l.liu@...el.com>,
        "Tian, Kevin" <kevin.tian@...el.com>,
        Raj Ashok <ashok.raj@...el.com>,
        Christoph Hellwig <hch@...radead.org>,
        Andriy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v4 20/22] iommu/vt-d: Add bind guest PASID support

Hi Jacob,

On 6/28/19 4:22 AM, Jacob Pan wrote:
>>> +		}
>>> +		refcount_set(&svm->refs, 0);
>>> +		ioasid_set_data(data->hpasid, svm);
>>> +		INIT_LIST_HEAD_RCU(&svm->devs);
>>> +		INIT_LIST_HEAD(&svm->list);
>>> +
>>> +		mmput(svm->mm);
>>> +	}
>>> +	sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
>>> +	if (!sdev) {
>>> +		ret = -ENOMEM;
>>> +		goto out;
>> I think you need to clean up svm if its device list is empty here, as
>> you said above:
>>
> No, we come here only if the device list is not empty and the new
> device to bind is different than any existing device in the list. If we
> cannot allocate memory for the new device, should not free the existing
> SVM, right?
> 

I'm sorry, but the code doesn't show this. We come here even an svm data
structure was newly created with an empty device list. I post the code
below to ensure that we are reading a same piece of code.

         mutex_lock(&pasid_mutex);
         svm = ioasid_find(NULL, data->hpasid, NULL);
         if (IS_ERR(svm)) {
                 ret = PTR_ERR(svm);
                 goto out;
         }
         if (svm) {
                 /*
                  * If we found svm for the PASID, there must be at
                  * least one device bond, otherwise svm should be freed.
                  */
                 BUG_ON(list_empty(&svm->devs));

                 for_each_svm_dev() {
                         /* In case of multiple sub-devices of the same 
pdev assigned, we should
                          * allow multiple bind calls with the same 
PASID and pdev.
                          */
                         sdev->users++;
                         goto out;
                 }
         } else {
                 /* We come here when PASID has never been bond to a 
device. */
                 svm = kzalloc(sizeof(*svm), GFP_KERNEL);
                 if (!svm) {
                         ret = -ENOMEM;
                         goto out;
                 }
                 /* REVISIT: upper layer/VFIO can track host process 
that bind the PASID.
                  * ioasid_set = mm might be sufficient for vfio to 
check pasid VMM
                  * ownership.
                  */
                 svm->mm = get_task_mm(current);
                 svm->pasid = data->hpasid;
                 if (data->flags & IOMMU_SVA_GPASID_VAL) {
                         svm->gpasid = data->gpasid;
                         svm->flags &= SVM_FLAG_GUEST_PASID;
                 }
                 refcount_set(&svm->refs, 0);
                 ioasid_set_data(data->hpasid, svm);
                 INIT_LIST_HEAD_RCU(&svm->devs);
                 INIT_LIST_HEAD(&svm->list);

                 mmput(svm->mm);
         }
         sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
         if (!sdev) {
                 ret = -ENOMEM;
                 goto out;
         }
         sdev->dev = dev;
         sdev->users = 1;

Best regards,
Baolu