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

[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.

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);
>