diff mbox series

drm/xe/display: fix potential overflow when multiplying 2 u32

Message ID 20240318110103.3872169-1-arun.r.murthy@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/xe/display: fix potential overflow when multiplying 2 u32 | expand

Commit Message

Arun R Murthy March 18, 2024, 11:01 a.m. UTC 
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(-)

Comments

Arun R Murthy March 28, 2024, 5:03 a.m. UTC | #1 
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
Arun R Murthy April 3, 2024, 3:39 p.m. UTC | #2 
Gentle Reminder!

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
Rodrigo Vivi April 3, 2024, 6:56 p.m. UTC | #3 
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
>
diff mbox series

Patch

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
+}