From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id EA3C710E0DB for ; Thu, 19 Oct 2023 15:23:26 +0000 (UTC) Message-ID: Date: Thu, 19 Oct 2023 17:23:22 +0200 MIME-Version: 1.0 Content-Language: en-US To: Kamil Konieczny , igt-dev@lists.freedesktop.org, zbigniew.kempczynski@intel.com, karolina.stolarek@intel.com References: <20231017143654.617723-1-marcin.bernatowicz@linux.intel.com> <20231018152807.46mc4l2ubrb5ttuv@kamilkon-desk.igk.intel.com> From: "Bernatowicz, Marcin" In-Reply-To: <20231018152807.46mc4l2ubrb5ttuv@kamilkon-desk.igk.intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 10/18/2023 5:28 PM, Kamil Konieczny wrote: > Hi Marcin, > On 2023-10-17 at 14:36:54 +0000, Marcin Bernatowicz wrote: >> Additionally check for overflow. > - ^^^^^^^^^^^^ > This type was from the start uint64, so imho change subject from: > > lib/intel_blt.c: ensure uint64_t result of multiplication > ------------ ^^ > sidenote: remove ".c" > > into: > lib/intel_blt: check for overflow in multiplication > > and adjust description. ensure 64-bit arithmetic multiplication ? > >> >> This should allow to exercise large buffers >> ex. xe_exercise_blt -W 16384 -H 16384 > > Please explain - this should fit in 32bit? 16K*16K*32 = 0x40000000 Given function blt_create_object(..., uint32_t width, uint32_t height, uint32_t bpp,..) I'm expecting correct uint64_t size calculation. uint64_t size = width * height * bpp / 8; > Or do you mean much higher values for W and H? whatever user provides ;) > >> >> Signed-off-by: Marcin Bernatowicz >> --- >> lib/intel_blt.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/lib/intel_blt.c b/lib/intel_blt.c >> index a76c7a404..f46c85e91 100644 >> --- a/lib/intel_blt.c >> +++ b/lib/intel_blt.c >> @@ -1607,12 +1607,18 @@ blt_create_object(const struct blt_copy_data *blt, uint32_t region, >> bool create_mapping) >> { >> struct blt_copy_object *obj; >> - uint64_t size = width * height * bpp / 8; >> uint32_t stride = tiling == T_LINEAR ? width * 4 : width; >> uint32_t handle; >> + uint64_t size; >> >> igt_assert_f(blt->driver, "Driver isn't set, have you called blt_copy_init()?\n"); >> >> + igt_assert_f((UINT64_MAX / 8) >= width && > ----------------- ^^^^^^^^^^^^^^ > This is not needed, it checks for MAX >= w * 8, while you want > size > 0, imho add a second assert after calculating size. There is no possibility of uint64_t overflow given uint32_t * uint32_t * uint32_t / 8 multiplication ? Should I remove this paranoid check? The most important is (uint64_t) cast in this patch to ensure 64-bit arithmetic. size > 0 may be an additional check, but do it twice ? (second one after broken ALIGN ;) -- marcin > > Regards, > Kamil > >> + (UINT64_MAX / width) >= height && >> + (UINT64_MAX / (width * height)) >= bpp, "Overflow detected!\n"); >> + >> + size = (uint64_t)width * height * bpp / 8; >> + >> obj = calloc(1, sizeof(*obj)); >> >> obj->size = size; >> -- >> 2.42.0 >>