* Re: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()
2024-03-27 13:41 [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid() Jason Gunthorpe
@ 2024-03-27 14:14 ` Yi Liu
2024-03-27 14:27 ` Jason Gunthorpe
2024-03-27 14:46 ` Yi Liu
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Yi Liu @ 2024-03-27 14:14 UTC (permalink / raw)
To: Jason Gunthorpe, iommu, Joerg Roedel, Robin Murphy, Will Deacon
Cc: Lu Baolu, Jean-Philippe Brucker, Joerg Roedel, Kevin Tian,
patches, Tony Zhu, Zhangfei Gao
On 2024/3/27 21:41, Jason Gunthorpe wrote:
> The SVA code checks that the PASID is valid for the device when assigning
> the PASID to the MM, but the normal PAGING related path does not check it.
> > Devices that don't support PASID or PASID values too large for the device
> should not invoke the driver callback. The drivers should rely on the
> core code for this enforcement.
I agree it is reasonable to enforce it in the core. But I'm not sure if a
fix tag is needed or not. As far as I know, intel iommu driver supports
attaching both the SVA and DMA type (PAGING) domain to pasid. Intel iommu
driver checks the max pasid in intel_pasid_get_entry() of
drivers/iommu/intel/pasid.c.
I'm not sure about ARM and AMD side, if the two drivers only support SVA
domain, and have the max pasid check. Then fix tag may be not necessary as
all the related paths are in good shape on the max pasid check before this
fix. :)
> Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu interfaces")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/iommu/iommu.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 098869007c69e5..a95a483def2d2a 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3354,6 +3354,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> {
> /* Caller must be a probed driver on dev */
> struct iommu_group *group = dev->iommu_group;
> + struct group_device *device;
> void *curr;
> int ret;
>
> @@ -3363,10 +3364,18 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> if (!group)
> return -ENODEV;
>
> - if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
> + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
> + pasid == IOMMU_NO_PASID)
perhaps this can be a separate patch as it means this API does not support
NO_PASID attachment.
> return -EINVAL;
>
> mutex_lock(&group->mutex);
> + for_each_group_device(group, device) {
> + if (pasid >= device->dev->iommu->max_pasids) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> + }
> +
> curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
> if (curr) {
> ret = xa_err(curr) ? : -EBUSY;
>
> base-commit: 4cece764965020c22cff7665b18a012006359095
Overall looks good to me although one nit.
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()
2024-03-27 14:14 ` Yi Liu
@ 2024-03-27 14:27 ` Jason Gunthorpe
2024-03-27 14:42 ` Yi Liu
0 siblings, 1 reply; 8+ messages in thread
From: Jason Gunthorpe @ 2024-03-27 14:27 UTC (permalink / raw)
To: Yi Liu
Cc: iommu, Joerg Roedel, Robin Murphy, Will Deacon, Lu Baolu,
Jean-Philippe Brucker, Joerg Roedel, Kevin Tian, patches,
Tony Zhu, Zhangfei Gao
On Wed, Mar 27, 2024 at 10:14:45PM +0800, Yi Liu wrote:
> On 2024/3/27 21:41, Jason Gunthorpe wrote:
> > The SVA code checks that the PASID is valid for the device when assigning
> > the PASID to the MM, but the normal PAGING related path does not check it.
> > > Devices that don't support PASID or PASID values too large for the device
> > should not invoke the driver callback. The drivers should rely on the
> > core code for this enforcement.
>
> I agree it is reasonable to enforce it in the core. But I'm not sure if a
> fix tag is needed or not. As far as I know, intel iommu driver supports
> attaching both the SVA and DMA type (PAGING) domain to pasid. Intel iommu
> driver checks the max pasid in intel_pasid_get_entry() of
> drivers/iommu/intel/pasid.c.
> I'm not sure about ARM and AMD side, if the two drivers only support SVA
> domain, and have the max pasid check. Then fix tag may be not necessary as
> all the related paths are in good shape on the max pasid check before this
> fix. :)
Ah, I could not find the max pasid check in the Intel driver.
> > Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu interfaces")
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> > drivers/iommu/iommu.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 098869007c69e5..a95a483def2d2a 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -3354,6 +3354,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> > {
> > /* Caller must be a probed driver on dev */
> > struct iommu_group *group = dev->iommu_group;
> > + struct group_device *device;
> > void *curr;
> > int ret;
> > @@ -3363,10 +3364,18 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> > if (!group)
> > return -ENODEV;
> > - if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
> > + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
> > + pasid == IOMMU_NO_PASID)
>
> perhaps this can be a separate patch as it means this API does not support
> NO_PASID attachment.
It never did? For something like Intel you can't use this API to
change the RID's domain, it would break things. It is all the same
topic - missing PASID validation.
That alone is worth the fixes :)
Thanks,
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()
2024-03-27 14:27 ` Jason Gunthorpe
@ 2024-03-27 14:42 ` Yi Liu
0 siblings, 0 replies; 8+ messages in thread
From: Yi Liu @ 2024-03-27 14:42 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: iommu, Joerg Roedel, Robin Murphy, Will Deacon, Lu Baolu,
Jean-Philippe Brucker, Joerg Roedel, Kevin Tian, patches,
Tony Zhu, Zhangfei Gao
On 2024/3/27 22:27, Jason Gunthorpe wrote:
> On Wed, Mar 27, 2024 at 10:14:45PM +0800, Yi Liu wrote:
>> On 2024/3/27 21:41, Jason Gunthorpe wrote:
>>> The SVA code checks that the PASID is valid for the device when assigning
>>> the PASID to the MM, but the normal PAGING related path does not check it.
>>>> Devices that don't support PASID or PASID values too large for the device
>>> should not invoke the driver callback. The drivers should rely on the
>>> core code for this enforcement.
>>
>> I agree it is reasonable to enforce it in the core. But I'm not sure if a
>> fix tag is needed or not. As far as I know, intel iommu driver supports
>> attaching both the SVA and DMA type (PAGING) domain to pasid. Intel iommu
>> driver checks the max pasid in intel_pasid_get_entry() of
>> drivers/iommu/intel/pasid.c.
>> I'm not sure about ARM and AMD side, if the two drivers only support SVA
>> domain, and have the max pasid check. Then fix tag may be not necessary as
>> all the related paths are in good shape on the max pasid check before this
>> fix. :)
>
> Ah, I could not find the max pasid check in the Intel driver.
I see. May have a look at the below code. When get pasid entry, it would
check the pasid against the return value of intel_pasid_get_dev_max_id(dev)
https://github.com/torvalds/linux/blob/7033999ecd7b8cf9ea59265035a0150961e023ee/drivers/iommu/intel/pasid.c#L137
>
>>> Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu interfaces")
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> ---
>>> drivers/iommu/iommu.c | 11 ++++++++++-
>>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 098869007c69e5..a95a483def2d2a 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -3354,6 +3354,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>> {
>>> /* Caller must be a probed driver on dev */
>>> struct iommu_group *group = dev->iommu_group;
>>> + struct group_device *device;
>>> void *curr;
>>> int ret;
>>> @@ -3363,10 +3364,18 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
>>> if (!group)
>>> return -ENODEV;
>>> - if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
>>> + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
>>> + pasid == IOMMU_NO_PASID)
>>
>> perhaps this can be a separate patch as it means this API does not support
>> NO_PASID attachment.
>
> It never did? For something like Intel you can't use this API to
> change the RID's domain, it would break things. It is all the same
> topic - missing PASID validation.
aha, yes. one patch is ok to me as well. :)
Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> That alone is worth the fixes :)
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()
2024-03-27 13:41 [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid() Jason Gunthorpe
2024-03-27 14:14 ` Yi Liu
@ 2024-03-27 14:46 ` Yi Liu
2024-03-27 16:37 ` Jason Gunthorpe
2024-03-28 3:23 ` Tian, Kevin
2024-03-28 5:40 ` Joerg Roedel
3 siblings, 1 reply; 8+ messages in thread
From: Yi Liu @ 2024-03-27 14:46 UTC (permalink / raw)
To: Jason Gunthorpe, iommu, Joerg Roedel, Robin Murphy, Will Deacon
Cc: Lu Baolu, Jean-Philippe Brucker, Joerg Roedel, Kevin Tian,
patches, Tony Zhu, Zhangfei Gao
On 2024/3/27 21:41, Jason Gunthorpe wrote:
> The SVA code checks that the PASID is valid for the device when assigning
> the PASID to the MM, but the normal PAGING related path does not check it.
>
> Devices that don't support PASID or PASID values too large for the device
> should not invoke the driver callback. The drivers should rely on the
> core code for this enforcement.
BTW. how about the iommu_detach_device_pasid(), should it also validate
pasid?
> Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu interfaces")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/iommu/iommu.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 098869007c69e5..a95a483def2d2a 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3354,6 +3354,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> {
> /* Caller must be a probed driver on dev */
> struct iommu_group *group = dev->iommu_group;
> + struct group_device *device;
> void *curr;
> int ret;
>
> @@ -3363,10 +3364,18 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
> if (!group)
> return -ENODEV;
>
> - if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
> + if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner ||
> + pasid == IOMMU_NO_PASID)
> return -EINVAL;
>
> mutex_lock(&group->mutex);
> + for_each_group_device(group, device) {
> + if (pasid >= device->dev->iommu->max_pasids) {
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> + }
> +
> curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
> if (curr) {
> ret = xa_err(curr) ? : -EBUSY;
>
> base-commit: 4cece764965020c22cff7665b18a012006359095
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()
2024-03-27 14:46 ` Yi Liu
@ 2024-03-27 16:37 ` Jason Gunthorpe
0 siblings, 0 replies; 8+ messages in thread
From: Jason Gunthorpe @ 2024-03-27 16:37 UTC (permalink / raw)
To: Yi Liu
Cc: iommu, Joerg Roedel, Robin Murphy, Will Deacon, Lu Baolu,
Jean-Philippe Brucker, Joerg Roedel, Kevin Tian, patches,
Tony Zhu, Zhangfei Gao
On Wed, Mar 27, 2024 at 10:46:35PM +0800, Yi Liu wrote:
> On 2024/3/27 21:41, Jason Gunthorpe wrote:
> > The SVA code checks that the PASID is valid for the device when assigning
> > the PASID to the MM, but the normal PAGING related path does not check it.
> >
> > Devices that don't support PASID or PASID values too large for the device
> > should not invoke the driver callback. The drivers should rely on the
> > core code for this enforcement.
>
> BTW. how about the iommu_detach_device_pasid(), should it also validate
> pasid?
Ultimately detach should check the xarray is !NULL and the xarray
entry cannot be non-NULL without passing the check. That is probably
something that should happen in your series adding the domain argument
to the removal...
Jason
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()
2024-03-27 13:41 [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid() Jason Gunthorpe
2024-03-27 14:14 ` Yi Liu
2024-03-27 14:46 ` Yi Liu
@ 2024-03-28 3:23 ` Tian, Kevin
2024-03-28 5:40 ` Joerg Roedel
3 siblings, 0 replies; 8+ messages in thread
From: Tian, Kevin @ 2024-03-28 3:23 UTC (permalink / raw)
To: Jason Gunthorpe, iommu@lists.linux.dev, Joerg Roedel,
Robin Murphy, Will Deacon
Cc: Lu Baolu, Jean-Philippe Brucker, Rodel, Jorg,
patches@lists.linux.dev, Zhu, Tony, Liu, Yi L, Zhangfei Gao
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, March 27, 2024 9:42 PM
>
> The SVA code checks that the PASID is valid for the device when assigning
> the PASID to the MM, but the normal PAGING related path does not check it.
>
> Devices that don't support PASID or PASID values too large for the device
> should not invoke the driver callback. The drivers should rely on the
> core code for this enforcement.
>
> Fixes: 16603704559c7a68 ("iommu: Add attach/detach_dev_pasid iommu
> interfaces")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid()
2024-03-27 13:41 [PATCH rc] iommu: Validate the PASID in iommu_attach_device_pasid() Jason Gunthorpe
` (2 preceding siblings ...)
2024-03-28 3:23 ` Tian, Kevin
@ 2024-03-28 5:40 ` Joerg Roedel
3 siblings, 0 replies; 8+ messages in thread
From: Joerg Roedel @ 2024-03-28 5:40 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: iommu, Robin Murphy, Will Deacon, Lu Baolu, Jean-Philippe Brucker,
Joerg Roedel, Kevin Tian, patches, Tony Zhu, Yi Liu, Zhangfei Gao
On Wed, Mar 27, 2024 at 10:41:39AM -0300, Jason Gunthorpe wrote:
> drivers/iommu/iommu.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
Applied, thanks.
^ permalink raw reply [flat|nested] 8+ messages in thread