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

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