[PATCH] drm/radeon/evergreen_cs: fix int overflow errors in cs track offsets
Christian König
christian.koenig at amd.com
Mon Aug 19 08:23:55 UTC 2024
Am 05.08.24 um 09:34 schrieb Nikita Zhandarovich:
>
> 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 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.
>>> 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?
Sorry I'm just back from vacation. Give me a week to dig through my
mails and talk with Alex.
Thanks,
Christian.
> 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 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