Re: [igt-dev] [PATCH i-g-t] lib/intel_blt.c: ensure uint64_t result of multiplication

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

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 <marcin.bernatowicz@linux.intel.com>
>> ---
>>   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
>>