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

Deucher, Alexander Alexander.Deucher at amd.com
Wed Aug 14 19:24:14 UTC 2024


[Public]

> -----Original Message-----
> From: Nikita Zhandarovich <n.zhandarovich at fintech.ru>
> Sent: Tuesday, August 6, 2024 1:19 PM
> To: Deucher, Alexander <Alexander.Deucher at amd.com>; Koenig, Christian
> <Christian.Koenig at amd.com>; Pan, Xinhui <Xinhui.Pan at amd.com>; David
> Airlie <airlied at gmail.com>; Daniel Vetter <daniel at ffwll.ch>
> Cc: Nikita Zhandarovich <n.zhandarovich at fintech.ru>; Jerome Glisse
> <jglisse at redhat.com>; Dave Airlie <airlied at redhat.com>; amd-
> gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; linux-
> kernel at vger.kernel.org; lvc-project at linuxtesting.org
> Subject: [PATCH v2] drm/radeon/evergreen_cs: fix int overflow errors in cs
> track offsets
>
> 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.
>
> Same goes for a few instances of 'surf.layer_size * mslice'
> multiplications that are added to 'offset' variable - they may potentially
> overflow as well and need to be validated properly.
>
> While some debug prints in this code section take possible overflow issues into
> account, simply casting to (unsigned long) may be erroneous in its own way,
> as depending on CPU architecture one is liable to get different results.
>
> Fix said problems by:
>  - casting 'offset' to fixed u64 data type instead of  ambiguous unsigned long.
>  - casting one of the operands in vulnerable to integer  overflow cases to u64.
>  - adjust format specifiers in debug prints to properly  represent 'offset' values.
>
> Found by Linux Verification Center (linuxtesting.org) with static analysis tool
> SVACE.
>
> Fixes: 285484e2d55e ("drm/radeon: add support for evergreen/ni tiling
> informations v11")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich at fintech.ru>

Applied.  Thanks!

Alex

> ---
> v2:
> - change data type to cast from unsigned long to u64 per Alex's and Christian's
> suggestion:
> https://lore.kernel.org/all/CADnq5_NaMr+vpqwqhsMoSeGrto2Lw5v0KXWEp
> 2HRK=++orScMg at mail.gmail.com/
> - include validation of surf.layer_size * mslice per Christian's
> approval:
> https://lore.kernel.org/all/1914cfcb-9700-4274-8120-
> 9746e241cb54 at amd.com/
> - change format specifiers when printing 'offset' value.
> - fix commit description to reflect patch changes.
>
> v1:
> https://lore.kernel.org/all/20240725180950.15820-1-
> n.zhandarovich at fintech.ru/
>
>  drivers/gpu/drm/radeon/evergreen_cs.c | 62 +++++++++++++++++-----------
> -------
>  1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen_cs.c
> b/drivers/gpu/drm/radeon/evergreen_cs.c
> index e5577d2a19ef..a46613283393 100644
> --- a/drivers/gpu/drm/radeon/evergreen_cs.c
> +++ b/drivers/gpu/drm/radeon/evergreen_cs.c
> @@ -397,7 +397,7 @@ static int evergreen_cs_track_validate_cb(struct
> radeon_cs_parser *p, unsigned i
>       struct evergreen_cs_track *track = p->track;
>       struct eg_surface surf;
>       unsigned pitch, slice, mslice;
> -     unsigned long offset;
> +     u64 offset;
>       int r;
>
>       mslice = G_028C6C_SLICE_MAX(track->cb_color_view[id]) + 1; @@ -
> 435,14 +435,14 @@ 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 = (u64)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",
> +             dev_warn(p->dev, "%s:%d cb[%d] bo base %llu not aligned
> with %ld\n",
>                        __func__, __LINE__, id, offset, surf.base_align);
>               return -EINVAL;
>       }
>
> -     offset += surf.layer_size * mslice;
> +     offset += (u64)surf.layer_size * mslice;
>       if (offset > radeon_bo_size(track->cb_color_bo[id])) {
>               /* old ddx are broken they allocate bo with w*h*bpp but
>                * program slice with ALIGN(h, 8), catch this and patch @@ -
> 450,14 +450,14 @@ static int evergreen_cs_track_validate_cb(struct
> radeon_cs_parser *p, unsigned i
>                */
>               if (!surf.mode) {
>                       uint32_t *ib = p->ib.ptr;
> -                     unsigned long tmp, nby, bsize, size, min = 0;
> +                     u64 tmp, nby, bsize, size, min = 0;
>
>                       /* find the height the ddx wants */
>                       if (surf.nby > 8) {
>                               min = surf.nby - 8;
>                       }
>                       bsize = radeon_bo_size(track->cb_color_bo[id]);
> -                     tmp = track->cb_color_bo_offset[id] << 8;
> +                     tmp = (u64)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) { @@ -469,7
> +469,7 @@ static int evergreen_cs_track_validate_cb(struct radeon_cs_parser
> *p, unsigned i
>                               slice = ((nby * surf.nbx) / 64) - 1;
>                               if (!evergreen_surface_check(p, &surf, "cb")) {
>                                       /* check if this one works */
> -                                     tmp += surf.layer_size * mslice;
> +                                     tmp += (u64)surf.layer_size * mslice;
>                                       if (tmp <= bsize) {
>                                               ib[track-
> >cb_color_slice_idx[id]] = slice;
>                                               goto old_ddx_ok;
> @@ -478,9 +478,9 @@ 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 %llu, max layer %d, bo size %ld, slice %d)\n",
>                        __func__, __LINE__, id, surf.layer_size,
> -                     track->cb_color_bo_offset[id] << 8, mslice,
> +                     (u64)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, @@ -564,7
> +564,7 @@ static int evergreen_cs_track_validate_stencil(struct
> radeon_cs_parser *p)
>       struct evergreen_cs_track *track = p->track;
>       struct eg_surface surf;
>       unsigned pitch, slice, mslice;
> -     unsigned long offset;
> +     u64 offset;
>       int r;
>
>       mslice = G_028008_SLICE_MAX(track->db_depth_view) + 1; @@ -
> 610,18 +610,18 @@ static int evergreen_cs_track_validate_stencil(struct
> radeon_cs_parser *p)
>               return r;
>       }
>
> -     offset = track->db_s_read_offset << 8;
> +     offset = (u64)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",
> +             dev_warn(p->dev, "%s:%d stencil read bo base %llu not
> aligned with
> +%ld\n",
>                        __func__, __LINE__, offset, surf.base_align);
>               return -EINVAL;
>       }
> -     offset += surf.layer_size * mslice;
> +     offset += (u64)surf.layer_size * mslice;
>       if (offset > radeon_bo_size(track->db_s_read_bo)) {
>               dev_warn(p->dev, "%s:%d stencil read bo too small (layer size
> %d, "
> -                      "offset %ld, max layer %d, bo size %ld)\n",
> +                      "offset %llu, max layer %d, bo size %ld)\n",
>                        __func__, __LINE__, surf.layer_size,
> -                     (unsigned long)track->db_s_read_offset << 8, mslice,
> +                     (u64)track->db_s_read_offset << 8, mslice,
>                       radeon_bo_size(track->db_s_read_bo));
>               dev_warn(p->dev, "%s:%d stencil invalid (0x%08x 0x%08x
> 0x%08x 0x%08x)\n",
>                        __func__, __LINE__, track->db_depth_size, @@ -
> 629,18 +629,18 @@ static int evergreen_cs_track_validate_stencil(struct
> radeon_cs_parser *p)
>               return -EINVAL;
>       }
>
> -     offset = track->db_s_write_offset << 8;
> +     offset = (u64)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",
> +             dev_warn(p->dev, "%s:%d stencil write bo base %llu not
> aligned with
> +%ld\n",
>                        __func__, __LINE__, offset, surf.base_align);
>               return -EINVAL;
>       }
> -     offset += surf.layer_size * mslice;
> +     offset += (u64)surf.layer_size * mslice;
>       if (offset > radeon_bo_size(track->db_s_write_bo)) {
>               dev_warn(p->dev, "%s:%d stencil write bo too small (layer size
> %d, "
> -                      "offset %ld, max layer %d, bo size %ld)\n",
> +                      "offset %llu, max layer %d, bo size %ld)\n",
>                        __func__, __LINE__, surf.layer_size,
> -                     (unsigned long)track->db_s_write_offset << 8, mslice,
> +                     (u64)track->db_s_write_offset << 8, mslice,
>                       radeon_bo_size(track->db_s_write_bo));
>               return -EINVAL;
>       }
> @@ -661,7 +661,7 @@ static int evergreen_cs_track_validate_depth(struct
> radeon_cs_parser *p)
>       struct evergreen_cs_track *track = p->track;
>       struct eg_surface surf;
>       unsigned pitch, slice, mslice;
> -     unsigned long offset;
> +     u64 offset;
>       int r;
>
>       mslice = G_028008_SLICE_MAX(track->db_depth_view) + 1; @@ -
> 708,34 +708,34 @@ static int evergreen_cs_track_validate_depth(struct
> radeon_cs_parser *p)
>               return r;
>       }
>
> -     offset = track->db_z_read_offset << 8;
> +     offset = (u64)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",
> +             dev_warn(p->dev, "%s:%d stencil read bo base %llu not
> aligned with
> +%ld\n",
>                        __func__, __LINE__, offset, surf.base_align);
>               return -EINVAL;
>       }
> -     offset += surf.layer_size * mslice;
> +     offset += (u64)surf.layer_size * mslice;
>       if (offset > radeon_bo_size(track->db_z_read_bo)) {
>               dev_warn(p->dev, "%s:%d depth read bo too small (layer size
> %d, "
> -                      "offset %ld, max layer %d, bo size %ld)\n",
> +                      "offset %llu, max layer %d, bo size %ld)\n",
>                        __func__, __LINE__, surf.layer_size,
> -                     (unsigned long)track->db_z_read_offset << 8, mslice,
> +                     (u64)track->db_z_read_offset << 8, mslice,
>                       radeon_bo_size(track->db_z_read_bo));
>               return -EINVAL;
>       }
>
> -     offset = track->db_z_write_offset << 8;
> +     offset = (u64)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",
> +             dev_warn(p->dev, "%s:%d stencil write bo base %llu not
> aligned with
> +%ld\n",
>                        __func__, __LINE__, offset, surf.base_align);
>               return -EINVAL;
>       }
> -     offset += surf.layer_size * mslice;
> +     offset += (u64)surf.layer_size * mslice;
>       if (offset > radeon_bo_size(track->db_z_write_bo)) {
>               dev_warn(p->dev, "%s:%d depth write bo too small (layer size
> %d, "
> -                      "offset %ld, max layer %d, bo size %ld)\n",
> +                      "offset %llu, max layer %d, bo size %ld)\n",
>                        __func__, __LINE__, surf.layer_size,
> -                     (unsigned long)track->db_z_write_offset << 8, mslice,
> +                     (u64)track->db_z_write_offset << 8, mslice,
>                       radeon_bo_size(track->db_z_write_bo));
>               return -EINVAL;
>       }


More information about the amd-gfx mailing list