| Message ID | 20230327232138.1490712-8-jacob.jun.pan@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Re-enable IDXD kernel workqueue under DMA API | expand |
On 3/28/23 7:21 AM, Jacob Pan wrote: > Devices that use ENQCMDS to submit work needs to retrieve its DMA > domain. It can then attach PASID to the DMA domain for shared mapping > (with RID) established by DMA API. > > Signed-off-by: Jacob Pan<jacob.jun.pan@linux.intel.com> > --- > drivers/iommu/iommu.c | 1 + > include/linux/iommu.h | 5 +++++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 10db680acaed..c51d343a75d2 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -2118,6 +2118,7 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev) > { > return dev->iommu_group->default_domain; > } > +EXPORT_SYMBOL_GPL(iommu_get_dma_domain); Directly exporting this function for external use seems unsafe. If the caller is the kernel driver for this device, it's fine because default domain remains unchanged during the life cycle of the driver. Otherwise, using this function may cause UAF. Keep in mind that group's default domain could be changed through sysfs. However, iommu_get_domain_for_dev() has already done so and has been exported. Maybe I'm worried too much. :-) Best regards, baolu
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Tuesday, March 28, 2023 2:04 PM > > On 3/28/23 7:21 AM, Jacob Pan wrote: > > Devices that use ENQCMDS to submit work needs to retrieve its DMA > > domain. It can then attach PASID to the DMA domain for shared mapping > > (with RID) established by DMA API. > > > > Signed-off-by: Jacob Pan<jacob.jun.pan@linux.intel.com> > > --- > > drivers/iommu/iommu.c | 1 + > > include/linux/iommu.h | 5 +++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > index 10db680acaed..c51d343a75d2 100644 > > --- a/drivers/iommu/iommu.c > > +++ b/drivers/iommu/iommu.c > > @@ -2118,6 +2118,7 @@ struct iommu_domain > *iommu_get_dma_domain(struct device *dev) > > { > > return dev->iommu_group->default_domain; > > } > > +EXPORT_SYMBOL_GPL(iommu_get_dma_domain); > > Directly exporting this function for external use seems unsafe. If the > caller is the kernel driver for this device, it's fine because default > domain remains unchanged during the life cycle of the driver. Otherwise, > using this function may cause UAF. Keep in mind that group's default > domain could be changed through sysfs. > > However, iommu_get_domain_for_dev() has already done so and has been > exported. Maybe I'm worried too much. :-) > Agree. The kernel driver managing the device wants to get the current domain of the device then iommu_get_domain_for_dev() is the right interface. It knows the domain is the dma domain.
Hi Kevin, On Tue, 28 Mar 2023 07:52:23 +0000, "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Baolu Lu <baolu.lu@linux.intel.com> > > Sent: Tuesday, March 28, 2023 2:04 PM > > > > On 3/28/23 7:21 AM, Jacob Pan wrote: > > > Devices that use ENQCMDS to submit work needs to retrieve its DMA > > > domain. It can then attach PASID to the DMA domain for shared mapping > > > (with RID) established by DMA API. > > > > > > Signed-off-by: Jacob Pan<jacob.jun.pan@linux.intel.com> > > > --- > > > drivers/iommu/iommu.c | 1 + > > > include/linux/iommu.h | 5 +++++ > > > 2 files changed, 6 insertions(+) > > > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > > index 10db680acaed..c51d343a75d2 100644 > > > --- a/drivers/iommu/iommu.c > > > +++ b/drivers/iommu/iommu.c > > > @@ -2118,6 +2118,7 @@ struct iommu_domain > > *iommu_get_dma_domain(struct device *dev) > > > { > > > return dev->iommu_group->default_domain; > > > } > > > +EXPORT_SYMBOL_GPL(iommu_get_dma_domain); > > > > Directly exporting this function for external use seems unsafe. If the > > caller is the kernel driver for this device, it's fine because default > > domain remains unchanged during the life cycle of the driver. Otherwise, > > using this function may cause UAF. Keep in mind that group's default > > domain could be changed through sysfs. > > > > However, iommu_get_domain_for_dev() has already done so and has been > > exported. Maybe I'm worried too much. :-) > > > > Agree. The kernel driver managing the device wants to get the current > domain of the device then iommu_get_domain_for_dev() is the right > interface. It knows the domain is the dma domain. This goes back to v1 then :) I thought the comments from v1 is that we don't want to check the domain type is DMA domain returned by iommu_get_domain_for_dev() If the driver "knows" the domain is dma domain, why can't it use iommu_get_dma_domain()? seems paradoxical. Thanks, Jacob
Hi Baolu, On Tue, 28 Mar 2023 14:04:15 +0800, Baolu Lu <baolu.lu@linux.intel.com> wrote: > On 3/28/23 7:21 AM, Jacob Pan wrote: > > Devices that use ENQCMDS to submit work needs to retrieve its DMA > > domain. It can then attach PASID to the DMA domain for shared mapping > > (with RID) established by DMA API. > > > > Signed-off-by: Jacob Pan<jacob.jun.pan@linux.intel.com> > > --- > > drivers/iommu/iommu.c | 1 + > > include/linux/iommu.h | 5 +++++ > > 2 files changed, 6 insertions(+) > > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > > index 10db680acaed..c51d343a75d2 100644 > > --- a/drivers/iommu/iommu.c > > +++ b/drivers/iommu/iommu.c > > @@ -2118,6 +2118,7 @@ struct iommu_domain *iommu_get_dma_domain(struct > > device *dev) { > > return dev->iommu_group->default_domain; > > } > > +EXPORT_SYMBOL_GPL(iommu_get_dma_domain); > > Directly exporting this function for external use seems unsafe. If the > caller is the kernel driver for this device, it's fine because default > domain remains unchanged during the life cycle of the driver. Otherwise, > using this function may cause UAF. Keep in mind that group's default > domain could be changed through sysfs. don't you have to unload the driver? > However, iommu_get_domain_for_dev() has already done so and has been > exported. Maybe I'm worried too much. :-) > > Best regards, > baolu Thanks, Jacob
On Tue, Mar 28, 2023 at 08:48:22AM -0700, Jacob Pan wrote: > > Agree. The kernel driver managing the device wants to get the current > > domain of the device then iommu_get_domain_for_dev() is the right > > interface. It knows the domain is the dma domain. > This goes back to v1 then :) > > I thought the comments from v1 is that we don't want to check the domain > type is DMA domain returned by iommu_get_domain_for_dev() > > If the driver "knows" the domain is dma domain, why can't it use > iommu_get_dma_domain()? seems paradoxical. Huh? The DMA API operates on the current domain of the device, be it IDENTITY or UNMANAGED. You have to copy whatever the current domain is to the PASID and you should definately not check if it is DMA or something. Jason
Hi Jason, On Tue, 28 Mar 2023 13:19:11 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote: > On Tue, Mar 28, 2023 at 08:48:22AM -0700, Jacob Pan wrote: > > > > Agree. The kernel driver managing the device wants to get the current > > > domain of the device then iommu_get_domain_for_dev() is the right > > > interface. It knows the domain is the dma domain. > > This goes back to v1 then :) > > > > I thought the comments from v1 is that we don't want to check the domain > > type is DMA domain returned by iommu_get_domain_for_dev() > > > > If the driver "knows" the domain is dma domain, why can't it use > > iommu_get_dma_domain()? seems paradoxical. > > Huh? > > The DMA API operates on the current domain of the device, be it > IDENTITY or UNMANAGED. > > You have to copy whatever the current domain is to the PASID and you > should definately not check if it is DMA or something. > right, the PASID works for IOVA, passthrough. I misunderstood the v1 review comments. I will go back to use iommu_get_domain_for_dev() but w/o checking domain types. Thanks all, Jacob
On 3/28/23 11:48 PM, Jacob Pan wrote: > On Tue, 28 Mar 2023 14:04:15 +0800, Baolu Lu<baolu.lu@linux.intel.com> > wrote: > >> On 3/28/23 7:21 AM, Jacob Pan wrote: >>> Devices that use ENQCMDS to submit work needs to retrieve its DMA >>> domain. It can then attach PASID to the DMA domain for shared mapping >>> (with RID) established by DMA API. >>> >>> Signed-off-by: Jacob Pan<jacob.jun.pan@linux.intel.com> >>> --- >>> drivers/iommu/iommu.c | 1 + >>> include/linux/iommu.h | 5 +++++ >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index 10db680acaed..c51d343a75d2 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -2118,6 +2118,7 @@ struct iommu_domain *iommu_get_dma_domain(struct >>> device *dev) { >>> return dev->iommu_group->default_domain; >>> } >>> +EXPORT_SYMBOL_GPL(iommu_get_dma_domain); >> Directly exporting this function for external use seems unsafe. If the >> caller is the kernel driver for this device, it's fine because default >> domain remains unchanged during the life cycle of the driver. Otherwise, >> using this function may cause UAF. Keep in mind that group's default >> domain could be changed through sysfs. > don't you have to unload the driver? Yes, of cause. Hence, the getting domain interfaces are only safe to be used in the driver of the device. Best regards, baolu
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 10db680acaed..c51d343a75d2 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -2118,6 +2118,7 @@ struct iommu_domain *iommu_get_dma_domain(struct device *dev) { return dev->iommu_group->default_domain; } +EXPORT_SYMBOL_GPL(iommu_get_dma_domain); /* * IOMMU groups are really the natural working unit of the IOMMU, but diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 0471089dc1d0..1ef9a1109534 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -1089,6 +1089,11 @@ iommu_get_domain_for_dev_pasid(struct device *dev, ioasid_t pasid, { return NULL; } + +static inline struct iommu_domain *iommu_get_dma_domain(struct device *dev) +{ + return NULL; +} #endif /* CONFIG_IOMMU_API */ /**
Devices that use ENQCMDS to submit work needs to retrieve its DMA domain. It can then attach PASID to the DMA domain for shared mapping (with RID) established by DMA API. Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com> --- drivers/iommu/iommu.c | 1 + include/linux/iommu.h | 5 +++++ 2 files changed, 6 insertions(+)