Re: [PATCH] drm/xe/display: fix potential overflow when multiplying 2 u32

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Apr 03, 2024 at 03:39:19PM +0000, Murthy, Arun R wrote:
> Gentle Reminder!

Thanks for your patch. I'm convinced we really need something like this.
At least to shout the static analyzers.

Or this or using the mul_u32_u32 or casting one of the right operands, otherwise the
result of the multiplication of a 32 vs 32 can overflow the 32 bits
before it is then moved to the u64 at the left. And this is undefined
behavior depending on the compiler and all.

But the commit message mentioning 'overflow' as it is kind of suggests
a true overflow issue on the result itself and a protection against that,
what is not true and likely kept the reviewers away from this patch.

Some commit message update like Himal did here [1] would be appreciated.
[1] https://patchwork.freedesktop.org/patch/586036/?series=131896&rev=1

> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Murthy,
> > Arun R
> > Sent: Thursday, March 28, 2024 10:34 AM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Subject: RE: [PATCH] drm/xe/display: fix potential overflow when multiplying 2
> > u32
> > 
> > Any comments?
> > 
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> > 
> > > -----Original Message-----
> > > From: Murthy, Arun R <arun.r.murthy@intel.com>
> > > Sent: Monday, March 18, 2024 4:31 PM
> > > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > > Cc: Murthy, Arun R <arun.r.murthy@intel.com>
> > > Subject: [PATCH] drm/xe/display: fix potential overflow when
> > > multiplying 2 u32
> > >
> > > Multiplying XE_PAGE_SIZE with another u32 and the product stored in
> > > u64 can potentially lead to overflow, use mul_u32_u32 instead.
> > >
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/display/xe_fb_pin.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > index 722c84a56607..e0b511ff7eab 100644
> > > --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > @@ -29,7 +29,7 @@ write_dpt_rotated(struct xe_bo *bo, struct iosys_map
> > > *map, u32 *dpt_ofs, u32 bo_
> > >  		u32 src_idx = src_stride * (height - 1) + column + bo_ofs;
> > >
> > >  		for (row = 0; row < height; row++) {
> > > -			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx *
> > > XE_PAGE_SIZE,
> > > +			u64 pte = ggtt->pt_ops->pte_encode_bo(bo,
> > > mul_u32_u32(src_idx,
> > > +XE_PAGE_SIZE),
> > >  							      xe-
> > > >pat.idx[XE_CACHE_WB]);
> > >
> > >  			iosys_map_wr(map, *dpt_ofs, u64, pte); @@ -61,7
> > > +61,7 @@ write_dpt_remapped(struct xe_bo *bo, struct iosys_map *map,
> > > +u32
> > > *dpt_ofs,
> > >
> > >  		for (column = 0; column < width; column++) {
> > >  			iosys_map_wr(map, *dpt_ofs, u64,
> > > -				     pte_encode_bo(bo, src_idx * XE_PAGE_SIZE,
> > > +				     pte_encode_bo(bo, mul_u32_u32(src_idx,
> > > XE_PAGE_SIZE),
> > >  				     xe->pat.idx[XE_CACHE_WB]));
> > >
> > >  			*dpt_ofs += 8;
> > > @@ -118,7 +118,7 @@ static int __xe_pin_fb_vma_dpt(struct
> > > intel_framebuffer *fb,
> > >  		u32 x;
> > >
> > >  		for (x = 0; x < size / XE_PAGE_SIZE; x++) {
> > > -			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x *
> > > XE_PAGE_SIZE,
> > > +			u64 pte = ggtt->pt_ops->pte_encode_bo(bo,
> > > mul_u32_u32(x,
> > > +XE_PAGE_SIZE),
> > >  							      xe-
> > > >pat.idx[XE_CACHE_WB]);
> > >
> > >  			iosys_map_wr(&dpt->vmap, x * 8, u64, pte); @@ -
> > > 164,7 +164,7 @@ write_ggtt_rotated(struct xe_bo *bo, struct xe_ggtt
> > > *ggtt,
> > > u32 *ggtt_ofs, u32 bo
> > >  		u32 src_idx = src_stride * (height - 1) + column + bo_ofs;
> > >
> > >  		for (row = 0; row < height; row++) {
> > > -			u64 pte = ggtt->pt_ops->pte_encode_bo(bo, src_idx *
> > > XE_PAGE_SIZE,
> > > +			u64 pte = ggtt->pt_ops->pte_encode_bo(bo,
> > > mul_u32_u32(src_idx,
> > > +XE_PAGE_SIZE),
> > >  							      xe-
> > > >pat.idx[XE_CACHE_WB]);
> > >
> > >  			xe_ggtt_set_pte(ggtt, *ggtt_ofs, pte); @@ -381,4
> > > +381,4 @@ struct i915_address_space *intel_dpt_create(struct
> > > intel_framebuffer *fb)  void intel_dpt_destroy(struct
> > > i915_address_space *vm) {
> > >  	return;
> > > -}
> > > \ No newline at end of file
> > > +}
> > > --
> > > 2.25.1
>