linux-kernel - RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID

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: <A2975661238FB949B60364EF0F2C25743A21C370@SHSMSX104.ccr.corp.intel.com>
Date:   Wed, 1 Apr 2020 05:48:52 +0000
From:   "Liu, Yi L" <yi.l.liu@...el.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "eric.auger@...hat.com" <eric.auger@...hat.com>
CC:     "jacob.jun.pan@...ux.intel.com" <jacob.jun.pan@...ux.intel.com>,
        "joro@...tes.org" <joro@...tes.org>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Tian, Jun J" <jun.j.tian@...el.com>,
        "Sun, Yi Y" <yi.y.sun@...el.com>,
        "jean-philippe@...aro.org" <jean-philippe@...aro.org>,
        "peterx@...hat.com" <peterx@...hat.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Wu, Hao" <hao.wu@...el.com>
Subject: RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)

> From: Tian, Kevin <kevin.tian@...el.com>
> Sent: Wednesday, April 1, 2020 1:43 PM
> To: Liu, Yi L <yi.l.liu@...el.com>; alex.williamson@...hat.com;
> Subject: RE: [PATCH v1 1/8] vfio: Add VFIO_IOMMU_PASID_REQUEST(alloc/free)
> 
> > From: Liu, Yi L <yi.l.liu@...el.com>
> > Sent: Tuesday, March 31, 2020 9:22 PM
> >
> > > From: Tian, Kevin <kevin.tian@...el.com>
> > > Sent: Tuesday, March 31, 2020 1:41 PM
> > > To: Liu, Yi L <yi.l.liu@...el.com>; alex.williamson@...hat.com;
> > > eric.auger@...hat.com
> > > Subject: RE: [PATCH v1 1/8] vfio: Add
> > VFIO_IOMMU_PASID_REQUEST(alloc/free)
> > >
> > > > From: Liu, Yi L <yi.l.liu@...el.com>
> > > > Sent: Monday, March 30, 2020 10:37 PM
> > > >
> > > > > From: Tian, Kevin <kevin.tian@...el.com>
> > > > > Sent: Monday, March 30, 2020 4:32 PM
> > > > > To: Liu, Yi L <yi.l.liu@...el.com>; alex.williamson@...hat.com;
> > > > > Subject: RE: [PATCH v1 1/8] vfio: Add
> > > > VFIO_IOMMU_PASID_REQUEST(alloc/free)
> > > > >
> > > > > > From: Liu, Yi L <yi.l.liu@...el.com>
> > > > > > Sent: Sunday, March 22, 2020 8:32 PM
> > > > > >
> > > > > > From: Liu Yi L <yi.l.liu@...el.com>
> > > > > >
> > > > > > For a long time, devices have only one DMA address space from
> > > > > > platform IOMMU's point of view. This is true for both bare
> > > > > > metal and directed- access in virtualization environment.
> > > > > > Reason is the source ID of DMA in PCIe are BDF (bus/dev/fnc
> > > > > > ID), which results in only device granularity
[...]
> > >
> > > >
> > > > > > +
> > > > > > +		switch (req.flags & VFIO_PASID_REQUEST_MASK) {
> > > > > > +		case VFIO_IOMMU_PASID_ALLOC:
> > > > > > +		{
> > > > > > +			int ret = 0, result;
> > > > > > +
> > > > > > +			result =
> > vfio_iommu_type1_pasid_alloc(iommu,
> > > > > > +
> > 	req.alloc_pasid.min,
> > > > > > +
> > 	req.alloc_pasid.max);
> > > > > > +			if (result > 0) {
> > > > > > +				offset = offsetof(
> > > > > > +					struct
> > > > > > vfio_iommu_type1_pasid_request,
> > > > > > +					alloc_pasid.result);
> > > > > > +				ret = copy_to_user(
> > > > > > +					      (void __user *) (arg +
> > offset),
> > > > > > +					      &result, sizeof(result));
> > > > > > +			} else {
> > > > > > +				pr_debug("%s: PASID alloc failed\n",
> > > > > > __func__);
> > > > > > +				ret = -EFAULT;
> > > > >
> > > > > no, this branch is not for copy_to_user error. it is about pasid
> > > > > alloc failure. you should handle both.
> > > >
> > > > Emmm, I just want to fail the IOCTL in such case, so the @result
> > > > field is meaningless in the user side. How about using another
> > > > return value (e.g. ENOSPC) to indicate the IOCTL failure?
> > >
> > > If pasid_alloc fails, you return its result to userspace if
> > > copy_to_user fails, then return -EFAULT.
> > >
> > > however, above you return -EFAULT for pasid_alloc failure, and then
> > > the number of not-copied bytes for copy_to_user.
> >
> > not quite get. Let me re-paste the code. :-)
> >
> > +		case VFIO_IOMMU_PASID_ALLOC:
> > +		{
> > +			int ret = 0, result;
> > +
> > +			result = vfio_iommu_type1_pasid_alloc(iommu,
> > +							req.alloc_pasid.min,
> > +							req.alloc_pasid.max);
> > +			if (result > 0) {
> > +				offset = offsetof(
> > +					struct
> > vfio_iommu_type1_pasid_request,
> > +					alloc_pasid.result);
> > +				ret = copy_to_user(
> > +					      (void __user *) (arg + offset),
> > +					      &result, sizeof(result));
> > if copy_to_user failed, ret is the number of uncopied bytes and will
> > be returned to userspace to indicate failure. userspace will not use
> > the data in result field. perhaps, I should check the ret here and
> > return -EFAULT or another errno, instead of return the number of
> > un-copied bytes.
> 
> here should return -EFAULT.

got it. so if any failure, the return value of this ioctl is a -ERROR_VAL.

> 
> > +			} else {
> > +				pr_debug("%s: PASID alloc failed\n",
> > __func__);
> > +				ret = -EFAULT;
> > if vfio_iommu_type1_pasid_alloc() failed, no doubt, return -EFAULT to
> > userspace to indicate failure.
> 
> pasid_alloc has its own error types returned. why blindly replace it with -EFAULT?

right, should use its own error types.

Thanks,
Yi Liu