[PATCH] drm/radeon/evergreen_cs: fix int overflow errors in cs track offsets
Christian König
christian.koenig at amd.com
Mon Jul 29 18:12:00 UTC 2024
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 at amd.com> 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 at vger.kernel.org
>>>>> 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.
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.
>
> 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 at fintech.ru>
>>>>>> ---
>>>>>> 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);
>
More information about the amd-gfx
mailing list