| Message ID | 20240806171904.49032-1-n.zhandarovich@fintech.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/radeon/evergreen_cs: fix int overflow errors in cs track offsets | expand |
Hi, On 8/6/24 10:19, Nikita Zhandarovich wrote: > 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@fintech.ru> > --- > 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+vpqwqhsMoSeGrto2Lw5v0KXWEp2HRK=++orScMg@mail.gmail.com/ > - include validation of surf.layer_size * mslice per Christian's > approval: > https://lore.kernel.org/all/1914cfcb-9700-4274-8120-9746e241cb54@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@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; > } Gentle ping... Regards, Nikita
[Public] > -----Original Message----- > From: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > Sent: Tuesday, August 6, 2024 1:19 PM > To: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig, Christian > <Christian.Koenig@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>; David > Airlie <airlied@gmail.com>; Daniel Vetter <daniel@ffwll.ch> > Cc: Nikita Zhandarovich <n.zhandarovich@fintech.ru>; Jerome Glisse > <jglisse@redhat.com>; Dave Airlie <airlied@redhat.com>; amd- > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; linux- > kernel@vger.kernel.org; lvc-project@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@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@mail.gmail.com/ > - include validation of surf.layer_size * mslice per Christian's > approval: > https://lore.kernel.org/all/1914cfcb-9700-4274-8120- > 9746e241cb54@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@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; > }
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; }
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@fintech.ru> --- 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+vpqwqhsMoSeGrto2Lw5v0KXWEp2HRK=++orScMg@mail.gmail.com/ - include validation of surf.layer_size * mslice per Christian's approval: https://lore.kernel.org/all/1914cfcb-9700-4274-8120-9746e241cb54@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@fintech.ru/ drivers/gpu/drm/radeon/evergreen_cs.c | 62 +++++++++++++++++------------------ 1 file changed, 31 insertions(+), 31 deletions(-)