[Intel-gfx] [PATCH 4/5] drm/i915: Fix overflows in fixed16_16

[Michal Wajdeczko]

On Fri, 22 Dec 2017 13:39:11 +0100, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-12-22 12:25:55)
>> If someone provides too large number for fixed16 type
>> we will WARN but we will not correctly clamp values
>> and that may lead to fully wrong calculations.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/fixed16_16.h | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/fixed16_16.h  
>> b/drivers/gpu/drm/i915/fixed16_16.h
>> index af23997..43fe0037 100644
>> --- a/drivers/gpu/drm/i915/fixed16_16.h
>> +++ b/drivers/gpu/drm/i915/fixed16_16.h
>> @@ -57,7 +57,8 @@ static inline fixed16_16_t u32_to_fixed16(u32 val)
>>  {
>>         fixed16_16_t fp;
>>
>> -       WARN_ON(val > U16_MAX);
>> +       if (WARN_ON(val > U16_MAX))
>> +               val = U16_MAX;
>
> return FIXED16_16_MAX;
>
> If val is too large, the closest we can get to val in our representation
> is 16_16_MAX.

hmm, true, but then our fixed representation will include fractional  
part...
but I guess it is still ok

>
>>
>>         fp.val = val << 16;
>>         return fp;
>
> return TO_FIXED16_16(val << 16);
>
>> @@ -93,7 +94,9 @@ static inline fixed16_16_t clamp_u64_to_fixed16(u64  
>> val)
>>  {
>>         fixed16_16_t fp;
>>
>> -       WARN_ON(val > U32_MAX);
>> +       if (WARN_ON(val > U32_MAX))
>> +               val = U32_MAX;
>
> I would do the early return. Perhaps check with gcc if prefers returning
> a constant than the jump back?
> -Chris