Re: [PATCH] drm/radeon/evergreen_cs: fix int overflow errors in cs track offsets

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 7/30/24 23:56, Christian König wrote:
> Am 30.07.24 um 19:36 schrieb Nikita Zhandarovich:
>> On 7/29/24 11:12, Christian König wrote:
>>> Am 29.07.24 um 20:04 schrieb Christian König:
>>>> Am 29.07.24 um 19:26 schrieb Nikita Zhandarovich:
>>>>> Hi,
>>>>>
>>>>> On 7/29/24 02:23, Christian König wrote:
>>>>>> Am 26.07.24 um 14:52 schrieb Alex Deucher:
>>>>>>> On Fri, Jul 26, 2024 at 3:05 AM Christian König
>>>>>>> <christian.koenig@xxxxxxx> wrote:
>>>>>>>> Am 25.07.24 um 20:09 schrieb Nikita Zhandarovich:
>>>>>>>>> Several cs track offsets (such as 'track->db_s_read_offset')
>>>>>>>>> either are initialized with or plainly take big enough values
>>>>>>>>> that,
>>>>>>>>> once shifted 8 bits left, may be hit with integer overflow if the
>>>>>>>>> resulting values end up going over u32 limit.
>>>>>>>>>
>>>>>>>>> Some debug prints take this into account (see according
>>>>>>>>> dev_warn() in
>>>>>>>>> evergreen_cs_track_validate_stencil()), even if the actual
>>>>>>>>> calculated value assigned to local 'offset' variable is missing
>>>>>>>>> similar proper expansion.
>>>>>>>>>
>>>>>>>>> Mitigate the problem by casting the type of right operands to the
>>>>>>>>> wider type of corresponding left ones in all such cases.
>>>>>>>>>
>>>>>>>>> Found by Linux Verification Center (linuxtesting.org) with static
>>>>>>>>> analysis tool SVACE.
>>>>>>>>>
>>>>>>>>> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni
>>>>>>>>> tiling informations v11")
>>>>>>>>> Cc: stable@xxxxxxxxxxxxxxx
>>>>>>>> Well first of all the long cast doesn't makes the value 64bit, it
>>>>>>>> depends on the architecture.
>>>>>>>>
>>>>>>>> Then IIRC the underlying hw can only handle a 32bit address
>>>>>>>> space so
>>>>>>>> having the offset as long is incorrect to begin with.
>>>>>>> Evergreen chips support a 36 bit internal address space and NI and
>>>>>>> newer support a 40 bit one, so this is applicable.
>>>>>> In that case I strongly suggest that we replace the unsigned long
>>>>>> with
>>>>>> u64 or otherwise we get different behavior on 32 and 64bit machines.
>>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>> To be clear, I'll prepare v2 patch that changes 'offset' to u64 as
>>>>> well
>>>>> as the cast of 'track->db_z_read_offset' (and the likes) to u64 too.
>>>>>
>>>>> On the other note, should I also include casting to wider type of the
>>>>> expression surf.layer_size * mslice (example down below) in
>>>>> evergreen_cs_track_validate_cb() and other similar functions? I can't
>>>>> properly gauge if the result will definitively fit into u32, maybe it
>>>>> makes sense to expand it as well?
>>>> The integer overflows caused by shifts are irrelevant and doesn't need
>>>> any fixing in the first place.
>>> Wait a second.
>>>
>>> Thinking more about it the integer overflows are actually necessary
>>> because that is exactly what happens in the hardware as well.
>>>
>>> If you don't overflow those shifts you actually create a security
>>> problem because the HW the might access at a different offset then you
>>> calculated here.
>>>
>>> We need to use something like a mask or use lower_32_bits() here.
>> Christian,
>>
>> My apologies, I may be getting a bit confused here.
>>
>> If integer overflows caused by shifts are predictable and constitute
>> normal behavior in this case, and there is no need to "fix" them, does
>> it still make sense to use any mitigations at all, i.e. masks or macros?
> 
> Well you stumbled over that somehow, so some automated checker things
> that this is a bad idea.
> 
>> Leaving these shifts to u32 variables as they are now will achieve the
>> same result as, for example, doing something along the lines of:
>>
>> offset = lower_32_bits((u64)track->cb_color_bo_offset[id] << 8);
>>
>> which seems clunky and unnecessary, even if it suppresses some static
>> analyzer triggers (and that seems overboard).
>> Or am I missing something obvious here?
> 
> No, it's just about suppressing the static checker warnings.
> 
> I'm also not 100% sure how that old hw works. Alex mentioned that it is
> using 36bits internally.
> 
> So it could be that we need to switch to using u64 here. I need to
> double check that with Alex.
> 
> But using unsigned long is certainly incorrect cause we then get
> different behavior based on the CPU architecture.
> 
> Thanks for pointing this out,
> Christian.
> 

Hi,

Christian, did you get a chance to go over hw specifics with Alex?
I'd really like to get on with v2 patch but I can't really start
properly if I don't know what (and how) exactly to fix.

I am also hesitant to split the fix into parts and I'd rather do the
whole int overflow mitigation in one set.

Thanks,
Nikita

>>> Regards,
>>> Christian.
>>>
>>>> The point is rather that we need to avoid multiplication overflows and
>>>> the security problems which come with those.
>>>>
>>>>> 441         }
>>>>> 442
>>>>> 443         offset += surf.layer_size * mslice;
>>>> In other words that here needs to be validated correctly.
>>>>
>> Agreed, I think either casting right operand to u64 (once 'offset' is
>> also changed from unsigned long to u64) or using mul_u32_u32() here and
>> in other places should suffice.
>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> 444         if (offset > radeon_bo_size(track->cb_color_bo[id])) {
>>>>> 445                 /* old ddx are broken they allocate bo with
>>>>> w*h*bpp
>>>>>
>>>>> Regards,
>>>>> Nikita
>>>>>>> Alex
>>>>>>>
>>>>>>>> And finally that is absolutely not material for stable.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Christian.
>>>>>>>>
>>>>>>>>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
>>>>>>>>> ---
>>>>>>>>> P.S. While I am not certain that track->cb_color_bo_offset[id]
>>>>>>>>> actually ends up taking values high enough to cause an overflow,
>>>>>>>>> nonetheless I thought it prudent to cast it to ulong as well.
>>>>>>>>>
>>>>>>>>>      drivers/gpu/drm/radeon/evergreen_cs.c | 18 +++++++++---------
>>>>>>>>>      1 file changed, 9 insertions(+), 9 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>> b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>> index 1fe6e0d883c7..d734d221e2da 100644
>>>>>>>>> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
>>>>>>>>> @@ -433,7 +433,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_cb(struct
>>>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>>>                  return r;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->cb_color_bo_offset[id] << 8;
>>>>>>>>> +     offset = (unsigned long)track->cb_color_bo_offset[id] << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d cb[%d] bo base %ld not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, id, offset,
>>>>>>>>> surf.base_align);
>>>>>>>>> @@ -455,7 +455,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_cb(struct
>>>>>>>>> radeon_cs_parser *p, unsigned i
>>>>>>>>>                                  min = surf.nby - 8;
>>>>>>>>>                          }
>>>>>>>>>                          bsize =
>>>>>>>>> radeon_bo_size(track->cb_color_bo[id]);
>>>>>>>>> -                     tmp = track->cb_color_bo_offset[id] << 8;
>>>>>>>>> +                     tmp = (unsigned
>>>>>>>>> long)track->cb_color_bo_offset[id] << 8;
>>>>>>>>>                          for (nby = surf.nby; nby > min; nby--) {
>>>>>>>>>                                  size = nby * surf.nbx *
>>>>>>>>> surf.bpe *
>>>>>>>>> surf.nsamples;
>>>>>>>>>                                  if ((tmp + size * mslice) <=
>>>>>>>>> bsize) {
>>>>>>>>> @@ -476,10 +476,10 @@ static int
>>>>>>>>> evergreen_cs_track_validate_cb(struct radeon_cs_parser *p,
>>>>>>>>> unsigned i
>>>>>>>>>                          }
>>>>>>>>>                  }
>>>>>>>>>                  dev_warn(p->dev, "%s:%d cb[%d] bo too small
>>>>>>>>> (layer
>>>>>>>>> size %d, "
>>>>>>>>> -                      "offset %d, max layer %d, bo size %ld,
>>>>>>>>> slice
>>>>>>>>> %d)\n",
>>>>>>>>> +                      "offset %ld, max layer %d, bo size %ld,
>>>>>>>>> slice
>>>>>>>>> %d)\n",
>>>>>>>>>                           __func__, __LINE__, id, surf.layer_size,
>>>>>>>>> -                     track->cb_color_bo_offset[id] << 8, mslice,
>>>>>>>>> - radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>>> +                     (unsigned long)track->cb_color_bo_offset[id]
>>>>>>>>> << 8,
>>>>>>>>> +                     mslice,
>>>>>>>>> radeon_bo_size(track->cb_color_bo[id]), slice);
>>>>>>>>>                  dev_warn(p->dev, "%s:%d problematic surf: (%d %d)
>>>>>>>>> (%d
>>>>>>>>> %d %d %d %d %d %d)\n",
>>>>>>>>>                           __func__, __LINE__, surf.nbx, surf.nby,
>>>>>>>>>                          surf.mode, surf.bpe, surf.nsamples,
>>>>>>>>> @@ -608,7 +608,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>>>                  return r;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->db_s_read_offset << 8;
>>>>>>>>> +     offset = (unsigned long)track->db_s_read_offset << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil read bo base
>>>>>>>>> %ld not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>>> surf.base_align);
>>>>>>>>> @@ -627,7 +627,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_stencil(struct radeon_cs_parser *p)
>>>>>>>>>                  return -EINVAL;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->db_s_write_offset << 8;
>>>>>>>>> +     offset = (unsigned long)track->db_s_write_offset << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>>>> not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>>> surf.base_align);
>>>>>>>>> @@ -706,7 +706,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>>>                  return r;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->db_z_read_offset << 8;
>>>>>>>>> +     offset = (unsigned long)track->db_z_read_offset << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil read bo base
>>>>>>>>> %ld not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>>> surf.base_align);
>>>>>>>>> @@ -722,7 +722,7 @@ static int
>>>>>>>>> evergreen_cs_track_validate_depth(struct radeon_cs_parser *p)
>>>>>>>>>                  return -EINVAL;
>>>>>>>>>          }
>>>>>>>>>
>>>>>>>>> -     offset = track->db_z_write_offset << 8;
>>>>>>>>> +     offset = (unsigned long)track->db_z_write_offset << 8;
>>>>>>>>>          if (offset & (surf.base_align - 1)) {
>>>>>>>>>                  dev_warn(p->dev, "%s:%d stencil write bo base %ld
>>>>>>>>> not
>>>>>>>>> aligned with %ld\n",
>>>>>>>>>                           __func__, __LINE__, offset,
>>>>>>>>> surf.base_align);
> 



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux