drm/i915/guc: prevent a possible int overflow in wq offsets

[PATCH] drm/i915/guc: prevent a possible int overflow in wq offsets

Posted by Nikita Zhandarovich 1 month, 2 weeks ago

It may be possible for the sum of the values derived from
i915_ggtt_offset() and __get_parent_scratch_offset()/
i915_ggtt_offset() to go over the u32 limit before being assigned
to wq offsets of u64 type.

Mitigate these issues by expanding one of the right operands
to u64 to avoid any overflow issues just in case.

Found by Linux Verification Center (linuxtesting.org) with static
analysis tool SVACE.

Fixes: 2584b3549f4c ("drm/i915/guc: Update to GuC version 70.1.1")
Cc: stable@vger.kernel.org
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 9400d0eb682b..908ebfa22933 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -2842,9 +2842,9 @@ static void prepare_context_registration_info_v70(struct intel_context *ce,
 		ce->parallel.guc.wqi_tail = 0;
 		ce->parallel.guc.wqi_head = 0;
 
-		wq_desc_offset = i915_ggtt_offset(ce->state) +
+		wq_desc_offset = (u64)i915_ggtt_offset(ce->state) +
 				 __get_parent_scratch_offset(ce);
-		wq_base_offset = i915_ggtt_offset(ce->state) +
+		wq_base_offset = (u64)i915_ggtt_offset(ce->state) +
 				 __get_wq_offset(ce);
 		info->wq_desc_lo = lower_32_bits(wq_desc_offset);
 		info->wq_desc_hi = upper_32_bits(wq_desc_offset);

Re: [PATCH] drm/i915/guc: prevent a possible int overflow in wq offsets

Posted by Rodrigo Vivi 1 day, 6 hours ago

On Thu, Jul 25, 2024 at 08:59:25AM -0700, Nikita Zhandarovich wrote:
> It may be possible for the sum of the values derived from
> i915_ggtt_offset() and __get_parent_scratch_offset()/
> i915_ggtt_offset() to go over the u32 limit before being assigned
> to wq offsets of u64 type.
> 
> Mitigate these issues by expanding one of the right operands
> to u64 to avoid any overflow issues just in case.
> 
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
> 
> Fixes: 2584b3549f4c ("drm/i915/guc: Update to GuC version 70.1.1")

this is not the correct fixes tag. The code is like this since

Fixes: c2aa552ff09d ("drm/i915/guc: Add multi-lrc context registration")
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>

Also, this is theoretical only since I don't believe it is actually
possible like Tvrtko had mentioned already.

But let's give a piece of mind to the static analyzers and get this in.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

(pushing now with the fixes fixes tag)

> Cc: stable@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 9400d0eb682b..908ebfa22933 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -2842,9 +2842,9 @@ static void prepare_context_registration_info_v70(struct intel_context *ce,
>  		ce->parallel.guc.wqi_tail = 0;
>  		ce->parallel.guc.wqi_head = 0;
>  
> -		wq_desc_offset = i915_ggtt_offset(ce->state) +
> +		wq_desc_offset = (u64)i915_ggtt_offset(ce->state) +
>  				 __get_parent_scratch_offset(ce);
> -		wq_base_offset = i915_ggtt_offset(ce->state) +
> +		wq_base_offset = (u64)i915_ggtt_offset(ce->state) +
>  				 __get_wq_offset(ce);
>  		info->wq_desc_lo = lower_32_bits(wq_desc_offset);
>  		info->wq_desc_hi = upper_32_bits(wq_desc_offset);

Re: [PATCH] drm/i915/guc: prevent a possible int overflow in wq offsets

Posted by Nikita Zhandarovich 1 week, 5 days ago

Hi,

On 7/25/24 08:59, Nikita Zhandarovich wrote:
> It may be possible for the sum of the values derived from
> i915_ggtt_offset() and __get_parent_scratch_offset()/
> i915_ggtt_offset() to go over the u32 limit before being assigned
> to wq offsets of u64 type.
> 
> Mitigate these issues by expanding one of the right operands
> to u64 to avoid any overflow issues just in case.
> 
> Found by Linux Verification Center (linuxtesting.org) with static
> analysis tool SVACE.
> 
> Fixes: 2584b3549f4c ("drm/i915/guc: Update to GuC version 70.1.1")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> index 9400d0eb682b..908ebfa22933 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> @@ -2842,9 +2842,9 @@ static void prepare_context_registration_info_v70(struct intel_context *ce,
>  		ce->parallel.guc.wqi_tail = 0;
>  		ce->parallel.guc.wqi_head = 0;
>  
> -		wq_desc_offset = i915_ggtt_offset(ce->state) +
> +		wq_desc_offset = (u64)i915_ggtt_offset(ce->state) +
>  				 __get_parent_scratch_offset(ce);
> -		wq_base_offset = i915_ggtt_offset(ce->state) +
> +		wq_base_offset = (u64)i915_ggtt_offset(ce->state) +
>  				 __get_wq_offset(ce);
>  		info->wq_desc_lo = lower_32_bits(wq_desc_offset);
>  		info->wq_desc_hi = upper_32_bits(wq_desc_offset);

Gentle ping,

Regards,
Nikita

Re: [PATCH] drm/i915/guc: prevent a possible int overflow in wq offsets

Posted by Tvrtko Ursulin 3 days, 15 hours ago

On 26/08/2024 11:45, Nikita Zhandarovich wrote:
> Hi,
> 
> On 7/25/24 08:59, Nikita Zhandarovich wrote:
>> It may be possible for the sum of the values derived from
>> i915_ggtt_offset() and __get_parent_scratch_offset()/
>> i915_ggtt_offset() to go over the u32 limit before being assigned
>> to wq offsets of u64 type.
>>
>> Mitigate these issues by expanding one of the right operands
>> to u64 to avoid any overflow issues just in case.
>>
>> Found by Linux Verification Center (linuxtesting.org) with static
>> analysis tool SVACE.
>>
>> Fixes: 2584b3549f4c ("drm/i915/guc: Update to GuC version 70.1.1")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> index 9400d0eb682b..908ebfa22933 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
>> @@ -2842,9 +2842,9 @@ static void prepare_context_registration_info_v70(struct intel_context *ce,
>>   		ce->parallel.guc.wqi_tail = 0;
>>   		ce->parallel.guc.wqi_head = 0;
>>   
>> -		wq_desc_offset = i915_ggtt_offset(ce->state) +
>> +		wq_desc_offset = (u64)i915_ggtt_offset(ce->state) +
>>   				 __get_parent_scratch_offset(ce);
>> -		wq_base_offset = i915_ggtt_offset(ce->state) +
>> +		wq_base_offset = (u64)i915_ggtt_offset(ce->state) +
>>   				 __get_wq_offset(ce);
>>   		info->wq_desc_lo = lower_32_bits(wq_desc_offset);
>>   		info->wq_desc_hi = upper_32_bits(wq_desc_offset);
> 
> Gentle ping,

With the current hardware this cannot overflow but I guess it doesn't 
harm to be explicitly safe. Adding some GuC folks to either r-b or add 
more candidates for review.

Regards,

Tvrtko