To be secure, when a userptr is invalidated the pages should be dma unmapped ensuring the device can no longer touch the invalidated pages.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate user pages") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: stable@vger.kernel.org # 6.8 Signed-off-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/xe/xe_vm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index dfd31b346021..964a5b4d47d8 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni, XE_WARN_ON(err); }
+ if (userptr->sg) + xe_hmm_userptr_free_sg(uvma); + trace_xe_vma_userptr_invalidate_complete(vma);
return true;
On Fri, Apr 26, 2024 at 04:32:36PM -0700, Matthew Brost wrote:
To be secure, when a userptr is invalidated the pages should be dma unmapped ensuring the device can no longer touch the invalidated pages.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate user pages") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: stable@vger.kernel.org # 6.8 Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/xe/xe_vm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index dfd31b346021..964a5b4d47d8 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni, XE_WARN_ON(err); }
- if (userptr->sg)
xe_hmm_userptr_free_sg(uvma);
I thought about this a bit, I think here we only dma unmap the SG, not free it. Freeing it could cause a current bind walk to access corrupt memory. Freeing can be deferred to the next attempt to bind the userptr or userptr destroy.
Matt
trace_xe_vma_userptr_invalidate_complete(vma); return true; -- 2.34.1
On Mon, 2024-04-29 at 01:11 +0000, Matthew Brost wrote:
On Fri, Apr 26, 2024 at 04:32:36PM -0700, Matthew Brost wrote:
To be secure, when a userptr is invalidated the pages should be dma unmapped ensuring the device can no longer touch the invalidated pages.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate user pages") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: stable@vger.kernel.org # 6.8 Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/xe/xe_vm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index dfd31b346021..964a5b4d47d8 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni, XE_WARN_ON(err); }
- if (userptr->sg)
xe_hmm_userptr_free_sg(uvma);I thought about this a bit, I think here we only dma unmap the SG, not free it. Freeing it could cause a current bind walk to access corrupt memory. Freeing can be deferred to the next attempt to bind the userptr or userptr destroy.
Yes, makes sense. /Thomas
Matt
trace_xe_vma_userptr_invalidate_complete(vma); return true; -- 2.34.1
Hi Matt
-----Original Message----- From: Intel-xe intel-xe-bounces@lists.freedesktop.org On Behalf Of Matthew Brost Sent: Friday, April 26, 2024 7:33 PM To: intel-xe@lists.freedesktop.org Cc: Brost, Matthew matthew.brost@intel.com; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
To be secure, when a userptr is invalidated the pages should be dma unmapped ensuring the device can no longer touch the invalidated pages.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate user pages") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: stable@vger.kernel.org # 6.8 Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/xe/xe_vm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index dfd31b346021..964a5b4d47d8 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni, XE_WARN_ON(err); }
- if (userptr->sg)
xe_hmm_userptr_free_sg(uvma);
Just some thoughts here. I think when we introduce system allocator, above should be made conditional. We should dma unmap userptr only for normal userptr but not for userptr created for system allocator (fault usrptr in the system allocator series). Because for system allocator the dma-unmapping would be part of the garbage collector and vma destroy process. Right?
Oak
trace_xe_vma_userptr_invalidate_complete(vma);
return true;
-- 2.34.1
On Mon, Apr 29, 2024 at 07:55:22AM -0600, Zeng, Oak wrote:
Hi Matt
-----Original Message----- From: Intel-xe intel-xe-bounces@lists.freedesktop.org On Behalf Of Matthew Brost Sent: Friday, April 26, 2024 7:33 PM To: intel-xe@lists.freedesktop.org Cc: Brost, Matthew matthew.brost@intel.com; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
To be secure, when a userptr is invalidated the pages should be dma unmapped ensuring the device can no longer touch the invalidated pages.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs") Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate user pages") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: stable@vger.kernel.org # 6.8 Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/xe/xe_vm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index dfd31b346021..964a5b4d47d8 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni, XE_WARN_ON(err); }
- if (userptr->sg)
xe_hmm_userptr_free_sg(uvma);Just some thoughts here. I think when we introduce system allocator, above should be made conditional. We should dma unmap userptr only for normal userptr but not for userptr created for system allocator (fault usrptr in the system allocator series). Because for system allocator the dma-unmapping would be part of the garbage collector and vma destroy process. Right?
I don't think it should be conditional. In any case when a CPU address is invalidated we need to ensure the dma mapping (IOMMU mapping) is also invalid to ensure no path to the old (invalidate) pages exists. This is an extra security that must be enforced. With removing the dma mapping, in theory rouge accesses from the GPU could still access the old pages.
Matt
Oak
trace_xe_vma_userptr_invalidate_complete(vma);
return true;
-- 2.34.1
-----Original Message----- From: Brost, Matthew matthew.brost@intel.com Sent: Monday, April 29, 2024 1:18 PM To: Zeng, Oak oak.zeng@intel.com Cc: intel-xe@lists.freedesktop.org; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: Re: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
On Mon, Apr 29, 2024 at 07:55:22AM -0600, Zeng, Oak wrote:
Hi Matt
-----Original Message----- From: Intel-xe intel-xe-bounces@lists.freedesktop.org On Behalf Of Matthew Brost Sent: Friday, April 26, 2024 7:33 PM To: intel-xe@lists.freedesktop.org Cc: Brost, Matthew matthew.brost@intel.com; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
To be secure, when a userptr is invalidated the pages should be dma unmapped ensuring the device can no longer touch the invalidated pages.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
GPUs")
Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate user pages") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: stable@vger.kernel.org # 6.8 Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/xe/xe_vm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index dfd31b346021..964a5b4d47d8 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni, XE_WARN_ON(err); }
- if (userptr->sg)
xe_hmm_userptr_free_sg(uvma);Just some thoughts here. I think when we introduce system allocator,
above should be made conditional. We should dma unmap userptr only for normal userptr but not for userptr created for system allocator (fault usrptr in the system allocator series). Because for system allocator the dma- unmapping would be part of the garbage collector and vma destroy process. Right?
I don't think it should be conditional. In any case when a CPU address is invalidated we need to ensure the dma mapping (IOMMU mapping) is also invalid to ensure no path to the old (invalidate) pages exists.
I understand for both normal userptr and fault userptr we need to dma unmap.
I was saying, for fault userptr, the dma unmap would be done in the garbage collector codes (we destroy fault userptr vma there and dma unmap along with vam destroy), so we don't need dma unmap in your above codes. It would something like this:
If (userptr && not fault userptr) Dma-unmap sg
If (fault userptr) Trigger garbage collector - this will deal with dma-unmap
Oak
This is an extra security that must be enforced. With removing the dma mapping, in theory rouge accesses from the GPU could still access the old pages.
Matt
Oak
trace_xe_vma_userptr_invalidate_complete(vma);
return true;
-- 2.34.1
On Tue, Apr 30, 2024 at 09:11:42AM -0600, Zeng, Oak wrote:
-----Original Message----- From: Brost, Matthew matthew.brost@intel.com Sent: Monday, April 29, 2024 1:18 PM To: Zeng, Oak oak.zeng@intel.com Cc: intel-xe@lists.freedesktop.org; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: Re: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
On Mon, Apr 29, 2024 at 07:55:22AM -0600, Zeng, Oak wrote:
Hi Matt
-----Original Message----- From: Intel-xe intel-xe-bounces@lists.freedesktop.org On Behalf Of Matthew Brost Sent: Friday, April 26, 2024 7:33 PM To: intel-xe@lists.freedesktop.org Cc: Brost, Matthew matthew.brost@intel.com; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
To be secure, when a userptr is invalidated the pages should be dma unmapped ensuring the device can no longer touch the invalidated pages.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
GPUs")
Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate user pages") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: stable@vger.kernel.org # 6.8 Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/xe/xe_vm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c index dfd31b346021..964a5b4d47d8 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni, XE_WARN_ON(err); }
- if (userptr->sg)
xe_hmm_userptr_free_sg(uvma);Just some thoughts here. I think when we introduce system allocator,
above should be made conditional. We should dma unmap userptr only for normal userptr but not for userptr created for system allocator (fault usrptr in the system allocator series). Because for system allocator the dma- unmapping would be part of the garbage collector and vma destroy process. Right?
I don't think it should be conditional. In any case when a CPU address is invalidated we need to ensure the dma mapping (IOMMU mapping) is also invalid to ensure no path to the old (invalidate) pages exists.
I understand for both normal userptr and fault userptr we need to dma unmap.
I was saying, for fault userptr, the dma unmap would be done in the garbage collector codes (we destroy fault userptr vma there and dma unmap along with vam destroy), so we don't need dma unmap in your above codes. It would something like this:
I understand what you are suggesting, but no this is always needed.
If (userptr && not fault userptr) Dma-unmap sg
With what you suggest, there is a window between the MMU notifier completing and garbage collector running in which the dma-mapping is valid. This is not allowed per the security model. Thus we need always invalidate dma-addresses in the notifier.
Matt
If (fault userptr) Trigger garbage collector - this will deal with dma-unmap
Oak
This is an extra security that must be enforced. With removing the dma mapping, in theory rouge accesses from the GPU could still access the old pages.
Matt
Oak
trace_xe_vma_userptr_invalidate_complete(vma);
return true;
-- 2.34.1
-----Original Message----- From: Brost, Matthew matthew.brost@intel.com Sent: Tuesday, April 30, 2024 11:42 AM To: Zeng, Oak oak.zeng@intel.com Cc: intel-xe@lists.freedesktop.org; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: Re: [PATCH] drm/xe: Unmap userptr in MMU invalidation notifier
On Tue, Apr 30, 2024 at 09:11:42AM -0600, Zeng, Oak wrote:
-----Original Message----- From: Brost, Matthew matthew.brost@intel.com Sent: Monday, April 29, 2024 1:18 PM To: Zeng, Oak oak.zeng@intel.com Cc: intel-xe@lists.freedesktop.org; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: Re: [PATCH] drm/xe: Unmap userptr in MMU invalidation
notifier
On Mon, Apr 29, 2024 at 07:55:22AM -0600, Zeng, Oak wrote:
Hi Matt
-----Original Message----- From: Intel-xe intel-xe-bounces@lists.freedesktop.org On Behalf
Of
Matthew Brost Sent: Friday, April 26, 2024 7:33 PM To: intel-xe@lists.freedesktop.org Cc: Brost, Matthew matthew.brost@intel.com; Thomas Hellström thomas.hellstrom@linux.intel.com; stable@vger.kernel.org Subject: [PATCH] drm/xe: Unmap userptr in MMU invalidation
notifier
To be secure, when a userptr is invalidated the pages should be dma unmapped ensuring the device can no longer touch the invalidated
pages.
Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel
GPUs")
Fixes: 12f4b58a37f4 ("drm/xe: Use hmm_range_fault to populate
user
pages") Cc: Thomas Hellström thomas.hellstrom@linux.intel.com Cc: stable@vger.kernel.org # 6.8 Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/xe/xe_vm.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/xe/xe_vm.c
b/drivers/gpu/drm/xe/xe_vm.c
index dfd31b346021..964a5b4d47d8 100644 --- a/drivers/gpu/drm/xe/xe_vm.c +++ b/drivers/gpu/drm/xe/xe_vm.c @@ -637,6 +637,9 @@ static bool vma_userptr_invalidate(struct mmu_interval_notifier *mni, XE_WARN_ON(err); }
- if (userptr->sg)
xe_hmm_userptr_free_sg(uvma);Just some thoughts here. I think when we introduce system allocator,
above should be made conditional. We should dma unmap userptr only
for
normal userptr but not for userptr created for system allocator (fault
usrptr
in the system allocator series). Because for system allocator the dma- unmapping would be part of the garbage collector and vma destroy
process.
Right?
I don't think it should be conditional. In any case when a CPU address is invalidated we need to ensure the dma mapping (IOMMU mapping) is also invalid to ensure no path to the old (invalidate) pages exists.
I understand for both normal userptr and fault userptr we need to dma
unmap.
I was saying, for fault userptr, the dma unmap would be done in the
garbage collector codes (we destroy fault userptr vma there and dma unmap along with vam destroy), so we don't need dma unmap in your above codes. It would something like this:
I understand what you are suggesting, but no this is always needed.
If (userptr && not fault userptr) Dma-unmap sg
With what you suggest, there is a window between the MMU notifier completing and garbage collector running in which the dma-mapping is valid. This is not allowed per the security model. Thus we need always invalidate dma-addresses in the notifier.
That make sense. Thanks for explain.
Oak
Matt
If (fault userptr) Trigger garbage collector - this will deal with dma-unmap
Oak
This is an extra security that must be enforced. With removing the dma mapping, in theory rouge accesses from the GPU could still access the old pages.
Matt
Oak
trace_xe_vma_userptr_invalidate_complete(vma);
return true;
-- 2.34.1
linux-stable-mirror@lists.linaro.org
-
Matthew Brost -
Thomas Hellström -
Zeng, Oak