From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from exchange.fintech.ru (exchange.fintech.ru [195.54.195.159]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 751CB1DA4C for ; Wed, 14 Aug 2024 13:49:33 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=195.54.195.159 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1723643379; cv=none; b=V3Qxeo56W4QKgEzwOjQDl22ysNj5kFyB/jLUeiAZBSgZQ8LtQkBZvilFfkJP8vCXRLijPywph7G6+6j5n4pZvRsCGWr+E67DCz6ivzqVAEV6jlHRauM7WIocmP9x4Z5JtTS0qISDfKRJgIK5V6WqAfQaNkJ2b/URaKmQuigfoPY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1723643379; c=relaxed/simple; bh=l10kmZPLkPbDhlhrpdpABXf/3ZxI/wfADpideBmkLNs=; h=Message-ID:Date:MIME-Version:Subject:To:CC:References:From: In-Reply-To:Content-Type; b=ODZOuZdqQpmKP1wzfJ2W0ekZLM63DEQQM0d25LBgZBZ2CCJvOCzmolV31XMC+sMpc9qbBF8okl1KeK3lOenwZc5X3aOpTwUtlBMIiKISc124q0nI4+gXXvadrmvvunEuAqHGWqhck3WMqDaeUj1vmAESbpToNfWauRbUeJQToS4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=fintech.ru; spf=pass smtp.mailfrom=fintech.ru; arc=none smtp.client-ip=195.54.195.159 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=fintech.ru Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=fintech.ru Received: from Ex16-01.fintech.ru (10.0.10.18) by exchange.fintech.ru (195.54.195.159) with Microsoft SMTP Server (TLS) id 14.3.498.0; Wed, 14 Aug 2024 16:48:20 +0300 Received: from [192.168.211.130] (10.0.253.138) by Ex16-01.fintech.ru (10.0.10.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Wed, 14 Aug 2024 16:48:20 +0300 Message-ID: <714daae5-b95c-425e-afe2-3b107033e493@fintech.ru> Date: Wed, 14 Aug 2024 06:48:14 -0700 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2] drm/radeon/evergreen_cs: fix int overflow errors in cs track offsets To: Alex Deucher , =?UTF-8?Q?Christian_K=C3=B6nig?= , Xinhui Pan , David Airlie , Daniel Vetter CC: Jerome Glisse , Dave Airlie , , , , , References: <20240806171904.49032-1-n.zhandarovich@fintech.ru> Content-Language: en-US From: Nikita Zhandarovich In-Reply-To: <20240806171904.49032-1-n.zhandarovich@fintech.ru> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-ClientProxiedBy: Ex16-02.fintech.ru (10.0.10.19) To Ex16-01.fintech.ru (10.0.10.18) 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 > --- > 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