[Intel-gfx] [PATCH v2] drm/i915: Fix a VMA UAF for multi-gt platform

[Intel-gfx] [PATCH v2] drm/i915: Fix a VMA UAF for multi-gt platform

[Nirmoy Das]

Ensure correct handling of closed VMAs on multi-gt platforms to prevent
Use-After-Free. Currently, when GT0 goes idle, closed VMAs that are
exclusively added to GT0's closed_vma link (gt->closed_vma) and
subsequently freed by i915_vma_parked(), which assumes the entire GPU is
idle. However, on platforms with multiple GTs, such as MTL, GT1 may
remain active while GT0 is idle. This causes GT0 to mistakenly consider
the closed VMAs in its closed_vma list as unnecessary, potentially
leading to Use-After-Free issues if a job for GT1 attempts to access a
freed VMA.

Although we do take a wakeref for GT0 but it happens later, after
evaluating VMAs. To mitigate this, it is necessary to hold a GT0 wakeref
early.

v2: Use gt id to detect multi-tile(Andi)
    Fix the incorrect error path.

Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Cc: Chris Wilson <chris.p.wilson@intel.com>
Cc: Andi Shyti <andi.shyti@linux.intel.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com>
Tested-by: Andi Shyti <andi.shyti@linux.intel.com>
Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 3aeede6aee4d..c2a67435acfa 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2683,6 +2683,7 @@ static int
 eb_select_engine(struct i915_execbuffer *eb)
 {
 	struct intel_context *ce, *child;
+	struct intel_gt *gt;
 	unsigned int idx;
 	int err;
 
@@ -2706,10 +2707,16 @@ eb_select_engine(struct i915_execbuffer *eb)
 		}
 	}
 	eb->num_batches = ce->parallel.number_children + 1;
+	gt = ce->engine->gt;
 
 	for_each_child(ce, child)
 		intel_context_get(child);
 	intel_gt_pm_get(ce->engine->gt);
+	/* Keep GT0 active on MTL so that i915_vma_parked() doesn't
+	 * free VMAs while execbuf ioctl is validating VMAs.
+	 */
+	if (gt->info.id)
+		intel_gt_pm_get(to_gt(gt->i915));
 
 	if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
 		err = intel_context_alloc_state(ce);
@@ -2748,6 +2755,9 @@ eb_select_engine(struct i915_execbuffer *eb)
 	return err;
 
 err:
+	if (gt->info.id)
+		intel_gt_pm_put(to_gt(gt->i915));
+
 	intel_gt_pm_put(ce->engine->gt);
 	for_each_child(ce, child)
 		intel_context_put(child);
@@ -2761,6 +2771,8 @@ eb_put_engine(struct i915_execbuffer *eb)
 	struct intel_context *child;
 
 	i915_vm_put(eb->context->vm);
+	if (eb->gt->info.id)
+		intel_gt_pm_put(to_gt(eb->gt->i915));
 	intel_gt_pm_put(eb->gt);
 	for_each_child(eb->context, child)
 		intel_context_put(child);
-- 
2.39.0

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix a VMA UAF for multi-gt platform

[Andi Shyti]

Hi Nirmoy,

On Tue, Jun 06, 2023 at 10:27:55PM +0200, Nirmoy Das wrote:
> Ensure correct handling of closed VMAs on multi-gt platforms to prevent
> Use-After-Free. Currently, when GT0 goes idle, closed VMAs that are
> exclusively added to GT0's closed_vma link (gt->closed_vma) and
> subsequently freed by i915_vma_parked(), which assumes the entire GPU is
> idle. However, on platforms with multiple GTs, such as MTL, GT1 may
> remain active while GT0 is idle. This causes GT0 to mistakenly consider
> the closed VMAs in its closed_vma list as unnecessary, potentially
> leading to Use-After-Free issues if a job for GT1 attempts to access a
> freed VMA.
> 
> Although we do take a wakeref for GT0 but it happens later, after
> evaluating VMAs. To mitigate this, it is necessary to hold a GT0 wakeref
> early.
> 
> v2: Use gt id to detect multi-tile(Andi)
>     Fix the incorrect error path.
> 
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Chris Wilson <chris.p.wilson@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com>
> Tested-by: Andi Shyti <andi.shyti@linux.intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>

I wonder if we need any Fixes tag here, maybe this:

Fixes: d93939730347 ("drm/i915: Remove the vma refcount")

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 3aeede6aee4d..c2a67435acfa 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2683,6 +2683,7 @@ static int
>  eb_select_engine(struct i915_execbuffer *eb)
>  {
>  	struct intel_context *ce, *child;
> +	struct intel_gt *gt;
>  	unsigned int idx;
>  	int err;
>  
> @@ -2706,10 +2707,16 @@ eb_select_engine(struct i915_execbuffer *eb)
>  		}
>  	}
>  	eb->num_batches = ce->parallel.number_children + 1;
> +	gt = ce->engine->gt;
>  
>  	for_each_child(ce, child)
>  		intel_context_get(child);
>  	intel_gt_pm_get(ce->engine->gt);
> +	/* Keep GT0 active on MTL so that i915_vma_parked() doesn't
> +	 * free VMAs while execbuf ioctl is validating VMAs.
> +	 */
> +	if (gt->info.id)
> +		intel_gt_pm_get(to_gt(gt->i915));
>  
>  	if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
>  		err = intel_context_alloc_state(ce);
> @@ -2748,6 +2755,9 @@ eb_select_engine(struct i915_execbuffer *eb)
>  	return err;
>  
>  err:
> +	if (gt->info.id)
> +		intel_gt_pm_put(to_gt(gt->i915));
> +
>  	intel_gt_pm_put(ce->engine->gt);
>  	for_each_child(ce, child)
>  		intel_context_put(child);
> @@ -2761,6 +2771,8 @@ eb_put_engine(struct i915_execbuffer *eb)
>  	struct intel_context *child;
>  
>  	i915_vm_put(eb->context->vm);
> +	if (eb->gt->info.id)
> +		intel_gt_pm_put(to_gt(eb->gt->i915));
>  	intel_gt_pm_put(eb->gt);

I would add a comment up here, just not to scare people when they
see this.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> 

Andi

>  	for_each_child(eb->context, child)
>  		intel_context_put(child);
> -- 
> 2.39.0

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Fix a VMA UAF for multi-gt platform (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5921 bytes --]

== Series Details ==

Series: drm/i915: Fix a VMA UAF for multi-gt platform (rev2)
URL   : https://patchwork.freedesktop.org/series/118887/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13238 -> Patchwork_118887v2
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/index.html

Participating hosts (41 -> 39)
------------------------------

  Missing    (2): bat-rpls-2 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_118887v2:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_addfb_basic@small-bo:
    - {bat-adlp-11}:      [PASS][1] -> [ABORT][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13238/bat-adlp-11/igt@kms_addfb_basic@small-bo.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-adlp-11/igt@kms_addfb_basic@small-bo.html

  
Known issues
------------

  Here are the changes found in Patchwork_118887v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@requests:
    - bat-rpls-1:         [PASS][3] -> [ABORT][4] ([i915#7911] / [i915#7920] / [i915#7982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13238/bat-rpls-1/igt@i915_selftest@live@requests.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-dg2-11:         NOTRUN -> [SKIP][5] ([i915#7828])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-dg2-11/igt@kms_chamelium_hpd@common-hpd-after-suspend.html
    - bat-adlm-1:         NOTRUN -> [SKIP][6] ([i915#7828])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-adlm-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1:
    - bat-dg2-8:          [PASS][7] -> [FAIL][8] ([i915#7932])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13238/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1.html

  * igt@kms_pipe_crc_basic@suspend-read-crc:
    - bat-adlm-1:         NOTRUN -> [SKIP][9] ([i915#1845])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-adlm-1/igt@kms_pipe_crc_basic@suspend-read-crc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - bat-dg2-11:         [ABORT][10] ([i915#7913] / [i915#7979]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13238/bat-dg2-11/igt@i915_selftest@live@hangcheck.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-dg2-11/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@workarounds:
    - bat-adlm-1:         [INCOMPLETE][12] ([i915#4983] / [i915#7677]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13238/bat-adlm-1/igt@i915_selftest@live@workarounds.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-adlm-1/igt@i915_selftest@live@workarounds.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1:
    - bat-dg2-8:          [FAIL][14] ([i915#7932]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13238/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-c-dp-1.html

  
#### Warnings ####

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-rplp-1:         [SKIP][16] ([i915#3555] / [i915#4579]) -> [ABORT][17] ([i915#4579] / [i915#8260])
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13238/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#7677]: https://gitlab.freedesktop.org/drm/intel/issues/7677
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7920]: https://gitlab.freedesktop.org/drm/intel/issues/7920
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7979]: https://gitlab.freedesktop.org/drm/intel/issues/7979
  [i915#7982]: https://gitlab.freedesktop.org/drm/intel/issues/7982
  [i915#8260]: https://gitlab.freedesktop.org/drm/intel/issues/8260


Build changes
-------------

  * Linux: CI_DRM_13238 -> Patchwork_118887v2

  CI-20190529: 20190529
  CI_DRM_13238: 8c0b302811d744b945dcb6d78164a76188914db9 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7320: 1c96b08a4cde6f2d49824a8cc3303bd860617b52 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_118887v2: 8c0b302811d744b945dcb6d78164a76188914db9 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

bfecf8522a41 drm/i915: Fix a VMA UAF for multi-gt platform

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_118887v2/index.html

[-- Attachment #2: Type: text/html, Size: 6959 bytes --]

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix a VMA UAF for multi-gt platform

[Andrzej Hajda]

On 06.06.2023 22:27, Nirmoy Das wrote:
> Ensure correct handling of closed VMAs on multi-gt platforms to prevent
> Use-After-Free. Currently, when GT0 goes idle, closed VMAs that are
> exclusively added to GT0's closed_vma link (gt->closed_vma) and
> subsequently freed by i915_vma_parked(), which assumes the entire GPU is
> idle. However, on platforms with multiple GTs, such as MTL, GT1 may
> remain active while GT0 is idle. This causes GT0 to mistakenly consider
> the closed VMAs in its closed_vma list as unnecessary, potentially
> leading to Use-After-Free issues if a job for GT1 attempts to access a
> freed VMA.
>
> Although we do take a wakeref for GT0 but it happens later, after
> evaluating VMAs. To mitigate this, it is necessary to hold a GT0 wakeref
> early.
>
> v2: Use gt id to detect multi-tile(Andi)
>      Fix the incorrect error path.
>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Chris Wilson <chris.p.wilson@intel.com>
> Cc: Andi Shyti <andi.shyti@linux.intel.com>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com>
> Tested-by: Andi Shyti <andi.shyti@linux.intel.com>
> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> ---
>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 3aeede6aee4d..c2a67435acfa 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -2683,6 +2683,7 @@ static int
>   eb_select_engine(struct i915_execbuffer *eb)
>   {
>   	struct intel_context *ce, *child;
> +	struct intel_gt *gt;
>   	unsigned int idx;
>   	int err;
>   
> @@ -2706,10 +2707,16 @@ eb_select_engine(struct i915_execbuffer *eb)
>   		}
>   	}
>   	eb->num_batches = ce->parallel.number_children + 1;
> +	gt = ce->engine->gt;
>   
>   	for_each_child(ce, child)
>   		intel_context_get(child);
>   	intel_gt_pm_get(ce->engine->gt);

intel_gt_pm_get(gt)


> +	/* Keep GT0 active on MTL so that i915_vma_parked() doesn't
> +	 * free VMAs while execbuf ioctl is validating VMAs.
> +	 */
> +	if (gt->info.id)
> +		intel_gt_pm_get(to_gt(gt->i915));
>   
>   	if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
>   		err = intel_context_alloc_state(ce);
> @@ -2748,6 +2755,9 @@ eb_select_engine(struct i915_execbuffer *eb)
>   	return err;
>   
>   err:
> +	if (gt->info.id)
> +		intel_gt_pm_put(to_gt(gt->i915));
> +
>   	intel_gt_pm_put(ce->engine->gt);

intel_gt_pm_put(gt)


Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>

Regards
Andrzej

>   	for_each_child(ce, child)
>   		intel_context_put(child);
> @@ -2761,6 +2771,8 @@ eb_put_engine(struct i915_execbuffer *eb)
>   	struct intel_context *child;
>   
>   	i915_vm_put(eb->context->vm);
> +	if (eb->gt->info.id)
> +		intel_gt_pm_put(to_gt(eb->gt->i915));
>   	intel_gt_pm_put(eb->gt);
>   	for_each_child(eb->context, child)
>   		intel_context_put(child);

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix a VMA UAF for multi-gt platform

[Nirmoy Das]

On 6/6/2023 10:56 PM, Andi Shyti wrote:
> Hi Nirmoy,
>
> On Tue, Jun 06, 2023 at 10:27:55PM +0200, Nirmoy Das wrote:
>> Ensure correct handling of closed VMAs on multi-gt platforms to prevent
>> Use-After-Free. Currently, when GT0 goes idle, closed VMAs that are
>> exclusively added to GT0's closed_vma link (gt->closed_vma) and
>> subsequently freed by i915_vma_parked(), which assumes the entire GPU is
>> idle. However, on platforms with multiple GTs, such as MTL, GT1 may
>> remain active while GT0 is idle. This causes GT0 to mistakenly consider
>> the closed VMAs in its closed_vma list as unnecessary, potentially
>> leading to Use-After-Free issues if a job for GT1 attempts to access a
>> freed VMA.
>>
>> Although we do take a wakeref for GT0 but it happens later, after
>> evaluating VMAs. To mitigate this, it is necessary to hold a GT0 wakeref
>> early.
>>
>> v2: Use gt id to detect multi-tile(Andi)
>>      Fix the incorrect error path.
>>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Chris Wilson <chris.p.wilson@intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com>
>> Tested-by: Andi Shyti <andi.shyti@linux.intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> I wonder if we need any Fixes tag here, maybe this:
>
> Fixes: d93939730347 ("drm/i915: Remove the vma refcount")

No, vma refcount is not enough unfortunately. Issue is i915_vma_parked() 
expects only one GT.


>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index 3aeede6aee4d..c2a67435acfa 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -2683,6 +2683,7 @@ static int
>>   eb_select_engine(struct i915_execbuffer *eb)
>>   {
>>   	struct intel_context *ce, *child;
>> +	struct intel_gt *gt;
>>   	unsigned int idx;
>>   	int err;
>>   
>> @@ -2706,10 +2707,16 @@ eb_select_engine(struct i915_execbuffer *eb)
>>   		}
>>   	}
>>   	eb->num_batches = ce->parallel.number_children + 1;
>> +	gt = ce->engine->gt;
>>   
>>   	for_each_child(ce, child)
>>   		intel_context_get(child);
>>   	intel_gt_pm_get(ce->engine->gt);
>> +	/* Keep GT0 active on MTL so that i915_vma_parked() doesn't
>> +	 * free VMAs while execbuf ioctl is validating VMAs.
>> +	 */
>> +	if (gt->info.id)
>> +		intel_gt_pm_get(to_gt(gt->i915));
>>   
>>   	if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
>>   		err = intel_context_alloc_state(ce);
>> @@ -2748,6 +2755,9 @@ eb_select_engine(struct i915_execbuffer *eb)
>>   	return err;
>>   
>>   err:
>> +	if (gt->info.id)
>> +		intel_gt_pm_put(to_gt(gt->i915));
>> +
>>   	intel_gt_pm_put(ce->engine->gt);
>>   	for_each_child(ce, child)
>>   		intel_context_put(child);
>> @@ -2761,6 +2771,8 @@ eb_put_engine(struct i915_execbuffer *eb)
>>   	struct intel_context *child;
>>   
>>   	i915_vm_put(eb->context->vm);
>> +	if (eb->gt->info.id)
>> +		intel_gt_pm_put(to_gt(eb->gt->i915));
>>   	intel_gt_pm_put(eb->gt);
> I would add a comment up here, just not to scare people when they
> see this.


I can add a comment pairing comment mentioning

eb_select_engine().

> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>


Thanks,

Nirmoy

>
> Andi
>
>>   	for_each_child(eb->context, child)
>>   		intel_context_put(child);
>> -- 
>> 2.39.0

Re: [Intel-gfx] [PATCH v2] drm/i915: Fix a VMA UAF for multi-gt platform

[Nirmoy Das]

On 6/7/2023 8:20 AM, Andrzej Hajda wrote:
>
>
> On 06.06.2023 22:27, Nirmoy Das wrote:
>> Ensure correct handling of closed VMAs on multi-gt platforms to prevent
>> Use-After-Free. Currently, when GT0 goes idle, closed VMAs that are
>> exclusively added to GT0's closed_vma link (gt->closed_vma) and
>> subsequently freed by i915_vma_parked(), which assumes the entire GPU is
>> idle. However, on platforms with multiple GTs, such as MTL, GT1 may
>> remain active while GT0 is idle. This causes GT0 to mistakenly consider
>> the closed VMAs in its closed_vma list as unnecessary, potentially
>> leading to Use-After-Free issues if a job for GT1 attempts to access a
>> freed VMA.
>>
>> Although we do take a wakeref for GT0 but it happens later, after
>> evaluating VMAs. To mitigate this, it is necessary to hold a GT0 wakeref
>> early.
>>
>> v2: Use gt id to detect multi-tile(Andi)
>>      Fix the incorrect error path.
>>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>> Cc: Chris Wilson <chris.p.wilson@intel.com>
>> Cc: Andi Shyti <andi.shyti@linux.intel.com>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com>
>> Tested-by: Andi Shyti <andi.shyti@linux.intel.com>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index 3aeede6aee4d..c2a67435acfa 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -2683,6 +2683,7 @@ static int
>>   eb_select_engine(struct i915_execbuffer *eb)
>>   {
>>       struct intel_context *ce, *child;
>> +    struct intel_gt *gt;
>>       unsigned int idx;
>>       int err;
>>   @@ -2706,10 +2707,16 @@ eb_select_engine(struct i915_execbuffer *eb)
>>           }
>>       }
>>       eb->num_batches = ce->parallel.number_children + 1;
>> +    gt = ce->engine->gt;
>>         for_each_child(ce, child)
>>           intel_context_get(child);
>>       intel_gt_pm_get(ce->engine->gt);
>
> intel_gt_pm_get(gt)
>
>
>> +    /* Keep GT0 active on MTL so that i915_vma_parked() doesn't
>> +     * free VMAs while execbuf ioctl is validating VMAs.
>> +     */
>> +    if (gt->info.id)
>> +        intel_gt_pm_get(to_gt(gt->i915));
>>         if (!test_bit(CONTEXT_ALLOC_BIT, &ce->flags)) {
>>           err = intel_context_alloc_state(ce);
>> @@ -2748,6 +2755,9 @@ eb_select_engine(struct i915_execbuffer *eb)
>>       return err;
>>     err:
>> +    if (gt->info.id)
>> +        intel_gt_pm_put(to_gt(gt->i915));
>> +
>>       intel_gt_pm_put(ce->engine->gt);
>
> intel_gt_pm_put(gt)
>
Will change both.
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>


Thanks,

Nirmoy

>
> Regards
> Andrzej
>
>>       for_each_child(ce, child)
>>           intel_context_put(child);
>> @@ -2761,6 +2771,8 @@ eb_put_engine(struct i915_execbuffer *eb)
>>       struct intel_context *child;
>>         i915_vm_put(eb->context->vm);
>> +    if (eb->gt->info.id)
>> +        intel_gt_pm_put(to_gt(eb->gt->i915));
>>       intel_gt_pm_put(eb->gt);
>>       for_each_child(eb->context, child)
>>           intel_context_put(child);
>