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

[Christian König <christian.koenig@xxxxxxx>]

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.

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