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?
441 }
442
443 offset += surf.layer_size * mslice;
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);
>
