Re: [Freedreno] [PATCH] drm/msm: fix an integer overflow test

[Jordan Crouse]

On Fri, Jun 30, 2017 at 10:59:15AM +0300, Dan Carpenter wrote:
> We recently added an integer overflow check but it needs an additional
> tweak to work properly on 32 bit systems.
> 
> The problem is that we're doing the right hand side of the assignment as
> type unsigned long so the max it will have an integer overflow instead
> of being larger than SIZE_MAX.  That means the "sz > SIZE_MAX" condition
> is never true even on 32 bit systems.  We need to first cast it to u64
> and then do the math.
> 
> Fixes: 4a630fadbb29 ("drm/msm: Fix potential buffer overflow issue")
> Signed-off-by: Dan Carpenter 

Indeed. Thanks for the catch.

Acked-by: Jordan Crouse 

> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c 
> b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 6bfca7470141..8095658e8cb4 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -34,8 +34,8 @@ static struct msm_gem_submit *submit_create(struct 
> drm_device *dev,
>   struct msm_gpu *gpu, uint32_t nr_bos, uint32_t nr_cmds)
>  {
>   struct msm_gem_submit *submit;
> - uint64_t sz = sizeof(*submit) + (nr_bos * sizeof(submit->bos[0])) +
> - (nr_cmds * sizeof(submit->cmd[0]));
> + uint64_t sz = sizeof(*submit) + ((u64)nr_bos * sizeof(submit->bos[0])) +
> + ((u64)nr_cmds * sizeof(submit->cmd[0]));
>  
>   if (sz > SIZE_MAX)
>   return NULL;
> ___
> Freedreno mailing list
> Freedreno@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/freedreno

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [Intel-gfx] [PATCH 1/2] drm/atomic: Change drm_atomic_helper_swap_state to return an error.

[Daniel Vetter]

On Wed, Jun 28, 2017 at 03:28:11PM +0200, Maarten Lankhorst wrote:
> We want to change swap_state to wait indefinitely, but to do this
> swap_state should wait interruptibly. This requires propagating
> the error to each driver. All drivers have changes to deal with the
> clean up. In order to allow easy reverting, the commit that changes
> behavior is separate so someone only has to revert that for testing.
> 
> Nouveau has a small bugfix, if drm_atomic_helper_wait_for_fences
> failed cleanup_planes was not called.
> 
> Cc: Boris Brezillon 
> Cc: David Airlie 
> Cc: Daniel Vetter 
> Cc: Jani Nikula 
> Cc: Sean Paul 
> Cc: CK Hu 
> Cc: Philipp Zabel 
> Cc: Matthias Brugger 
> Cc: Rob Clark 
> Cc: Ben Skeggs 
> Cc: Thierry Reding 
> Cc: Jonathan Hunter 
> Cc: Jyri Sarha 
> Cc: Tomi Valkeinen 
> Cc: Eric Anholt 
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-ker...@vger.kernel.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: linux-te...@vger.kernel.org
> Signed-off-by: Maarten Lankhorst 

We kinda need to backport this to older kernels, but I don't really see
how :( Maybe we should split this up:
patch 1: Change to int return type
patches 2-(n-1): Driver conversions
patch n: __must_check addition

That would at least somewhat make this backportable ...

> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 14 --
>  drivers/gpu/drm/drm_atomic_helper.c  | 18 --
>  drivers/gpu/drm/i915/intel_display.c | 10 +-
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c   |  7 ++-
>  drivers/gpu/drm/msm/msm_atomic.c | 14 +-
>  drivers/gpu/drm/nouveau/nv50_display.c   | 10 --
>  drivers/gpu/drm/tegra/drm.c  |  7 ++-
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c  |  6 +-
>  drivers/gpu/drm/vc4/vc4_kms.c| 21 +
>  include/drm/drm_atomic_helper.h  |  4 ++--
>  10 files changed, 82 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c 
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 516d9547d331..d4f787bf1d4a 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -544,9 +544,11 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
> drm_device *dev,
>   goto error;
>   }
>  
> - /* Swap the state, this is the point of no return. */
> - drm_atomic_helper_swap_state(state, true);

Push the swap_state up over the commit setup (but after the allocation)
and there's no more a problem with unrolling.

> + ret = drm_atomic_helper_swap_state(state, true);
> + if (ret)
> + goto err_pending;
>  
> + /* Swap state succeeded, this is the point of no return. */
>   drm_atomic_state_get(state);
>   if (async)
>   queue_work(dc->wq, >work);
> @@ -555,6 +557,14 @@ static int atmel_hlcdc_dc_atomic_commit(struct 
> drm_device *dev,
>  
>   return 0;
>  
> +err_pending:
> + /* Fail the commit, wake up any waiter. */
> + spin_lock(>commit.wait.lock);
> + dc->commit.pending = false;
> + wake_up_all_locked(>commit.wait);
> + spin_unlock(>commit.wait.lock);
> +err_free:
> + kfree(commit);
>  error:
>   drm_atomic_helper_cleanup_planes(dev, state);
>   return ret;
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index f1847d29ba3e..f66b6c45cdd0 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1387,10 +1387,8 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>  
>   if (!nonblock) {
>   ret = drm_atomic_helper_wait_for_fences(dev, state, true);
> - if (ret) {
> - drm_atomic_helper_cleanup_planes(dev, state);
> - return ret;
> - }
> + if (ret)
> + goto err;
>   }
>  
>   /*
> @@ -1399,7 +1397,9 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>* the software side now.
>*/
>  
> - drm_atomic_helper_swap_state(state, true);
> + ret = drm_atomic_helper_swap_state(state, true);
> + if (ret)
> + goto err;
>  
>   /*
>* Everything below can be run asynchronously without the need to grab
> @@ -1428,6 +1428,10 @@ int drm_atomic_helper_commit(struct 

[Freedreno] [PATCH] drm/msm: fix an integer overflow test

[Dan Carpenter]

We recently added an integer overflow check but it needs an additional
tweak to work properly on 32 bit systems.

The problem is that we're doing the right hand side of the assignment as
type unsigned long so the max it will have an integer overflow instead
of being larger than SIZE_MAX.  That means the "sz > SIZE_MAX" condition
is never true even on 32 bit systems.  We need to first cast it to u64
and then do the math.

Fixes: 4a630fadbb29 ("drm/msm: Fix potential buffer overflow issue")
Signed-off-by: Dan Carpenter 

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c 
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 6bfca7470141..8095658e8cb4 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -34,8 +34,8 @@ static struct msm_gem_submit *submit_create(struct drm_device 
*dev,
struct msm_gpu *gpu, uint32_t nr_bos, uint32_t nr_cmds)
 {
struct msm_gem_submit *submit;
-   uint64_t sz = sizeof(*submit) + (nr_bos * sizeof(submit->bos[0])) +
-   (nr_cmds * sizeof(submit->cmd[0]));
+   uint64_t sz = sizeof(*submit) + ((u64)nr_bos * sizeof(submit->bos[0])) +
+   ((u64)nr_cmds * sizeof(submit->cmd[0]));
 
if (sz > SIZE_MAX)
return NULL;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno