diff mbox series

drm/komeda: Fix bit check to import to value of proper type

Message ID 20210127123430.301752-1-carsten.haitzler@foss.arm.com (mailing list archive)
State New, archived
Headers show
Series drm/komeda: Fix bit check to import to value of proper type | expand

Commit Message

Carsten Haitzler Jan. 27, 2021, 12:34 p.m. UTC 
From: Carsten Haitzler <carsten.haitzler@arm.com>

Another issue found by KASAN. The bit finding is buried inside the
dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
calls the bit stuff. These bit functions want an unsigned long pointer
as input and just dumbly casting leads to out-of-bounds accesses.
This fixes that.

Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
---
 .../gpu/drm/arm/display/include/malidp_utils.h   | 10 ++++++++--
 .../gpu/drm/arm/display/komeda/komeda_pipeline.c | 16 +++++++++++-----
 .../arm/display/komeda/komeda_pipeline_state.c   | 13 ++++++++-----
 3 files changed, 27 insertions(+), 12 deletions(-)

Comments

Steven Price Jan. 27, 2021, 4:31 p.m. UTC | #1 
NIT: This is the second version of this patch so should have "[PATCH 
v2]" in the subject.

On 27/01/2021 12:34, carsten.haitzler@foss.arm.com wrote:
> From: Carsten Haitzler <carsten.haitzler@arm.com>
> 
> Another issue found by KASAN. The bit finding is buried inside the
> dp_for_each_set_bit() macro (that passes on to for_each_set_bit() that
> calls the bit stuff. These bit functions want an unsigned long pointer
> as input and just dumbly casting leads to out-of-bounds accesses.
> This fixes that.
> 
> Signed-off-by: Carsten Haitzler <carsten.haitzler@arm.com>
> ---
>   .../gpu/drm/arm/display/include/malidp_utils.h   | 10 ++++++++--
>   .../gpu/drm/arm/display/komeda/komeda_pipeline.c | 16 +++++++++++-----
>   .../arm/display/komeda/komeda_pipeline_state.c   | 13 ++++++++-----
>   3 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h b/drivers/gpu/drm/arm/display/include/malidp_utils.h
> index 3bc383d5bf73..8d289cd0b5b8 100644
> --- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
> +++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
> @@ -12,9 +12,15 @@
>   
>   #define has_bit(nr, mask)	(BIT(nr) & (mask))
>   #define has_bits(bits, mask)	(((bits) & (mask)) == (bits))
> -
> +/*
> +#define dp_for_each_set_bit(bit, mask) \
> +	for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
> +#define dp_for_each_set_bit(bit, mask) \
> +	unsigned long __local_mask = mask; \
> +	for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
> +*/

Commented out code left in - please remove it.

>   #define dp_for_each_set_bit(bit, mask) \
> -	for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
> +	for_each_set_bit((bit), &(mask), sizeof(mask) * 8)

I'm not really sure if there's much point in this macro now. In practice 
the uses below are now getting the wrong length (because sizeof(mask) == 
sizeof(unsigned long) ) but we actually know the size is smaller in most 
cases, so we could pass a more appropriate value in.

Other than that the changes below look correct to me.

Steve

>   
>   #define dp_wait_cond(__cond, __tries, __min_range, __max_range)	\
>   ({							\
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> index 719a79728e24..a85c8a806334 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
> @@ -46,8 +46,9 @@ void komeda_pipeline_destroy(struct komeda_dev *mdev,
>   {
>   	struct komeda_component *c;
>   	int i;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
> -	dp_for_each_set_bit(i, pipe->avail_comps) {
> +	dp_for_each_set_bit(i, avail_comps) {
>   		c = komeda_pipeline_get_component(pipe, i);
>   		komeda_component_destroy(mdev, c);
>   	}
> @@ -247,6 +248,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
>   {
>   	struct komeda_component *c;
>   	int id;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
>   	DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
>   		 pipe->id, pipe->n_layers, pipe->n_scalers,
> @@ -258,7 +260,7 @@ static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
>   		 pipe->of_output_links[1] ?
>   		 pipe->of_output_links[1]->full_name : "none");
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	dp_for_each_set_bit(id, avail_comps) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   
>   		komeda_component_dump(c);
> @@ -270,8 +272,9 @@ static void komeda_component_verify_inputs(struct komeda_component *c)
>   	struct komeda_pipeline *pipe = c->pipeline;
>   	struct komeda_component *input;
>   	int id;
> +	unsigned long supported_inputs = c->supported_inputs;
>   
> -	dp_for_each_set_bit(id, c->supported_inputs) {
> +	dp_for_each_set_bit(id, supported_inputs) {
>   		input = komeda_pipeline_get_component(pipe, id);
>   		if (!input) {
>   			c->supported_inputs &= ~(BIT(id));
> @@ -302,8 +305,9 @@ static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
>   	struct komeda_component *c;
>   	struct komeda_layer *layer;
>   	int i, id;
> +	unsigned long avail_comps = pipe->avail_comps;
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	dp_for_each_set_bit(id, avail_comps) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   		komeda_component_verify_inputs(c);
>   	}
> @@ -355,13 +359,15 @@ void komeda_pipeline_dump_register(struct komeda_pipeline *pipe,
>   {
>   	struct komeda_component *c;
>   	u32 id;
> +	unsigned long avail_comps;
>   
>   	seq_printf(sf, "\n======== Pipeline-%d ==========\n", pipe->id);
>   
>   	if (pipe->funcs && pipe->funcs->dump_register)
>   		pipe->funcs->dump_register(pipe, sf);
>   
> -	dp_for_each_set_bit(id, pipe->avail_comps) {
> +	avail_comps = pipe->avail_comps;
> +	dp_for_each_set_bit(id, avail_comps) {
>   		c = komeda_pipeline_get_component(pipe, id);
>   
>   		seq_printf(sf, "\n------%s------\n", c->name);
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> index e8b1e15312d8..7640dae7f4bf 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
> @@ -1232,7 +1232,8 @@ komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
>   	struct komeda_component_state *c_st;
>   	struct komeda_component *c;
> -	u32 disabling_comps, id;
> +	u32 id;
> +	unsigned long disabling_comps;
>   
>   	WARN_ON(!old);
>   
> @@ -1287,7 +1288,8 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *old;
>   	struct komeda_component *c;
>   	struct komeda_component_state *c_st;
> -	u32 id, disabling_comps = 0;
> +	u32 id;
> +	unsigned long disabling_comps;
>   
>   	old = komeda_pipeline_get_old_state(pipe, old_state);
>   
> @@ -1297,7 +1299,7 @@ bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
>   		disabling_comps = old->active_comps &
>   				  pipe->standalone_disabled_comps;
>   
> -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
>   			 pipe->id, old->active_comps, disabling_comps);
>   
>   	dp_for_each_set_bit(id, disabling_comps) {
> @@ -1331,13 +1333,14 @@ void komeda_pipeline_update(struct komeda_pipeline *pipe,
>   	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
>   	struct komeda_pipeline_state *old;
>   	struct komeda_component *c;
> -	u32 id, changed_comps = 0;
> +	u32 id;
> +	unsigned long changed_comps;
>   
>   	old = komeda_pipeline_get_old_state(pipe, old_state);
>   
>   	changed_comps = new->active_comps | old->active_comps;
>   
> -	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
> +	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
>   			 pipe->id, new->active_comps, changed_comps);
>   
>   	dp_for_each_set_bit(id, changed_comps) {
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h b/drivers/gpu/drm/arm/display/include/malidp_utils.h
index 3bc383d5bf73..8d289cd0b5b8 100644
--- a/drivers/gpu/drm/arm/display/include/malidp_utils.h
+++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h
@@ -12,9 +12,15 @@ 
 
 #define has_bit(nr, mask)	(BIT(nr) & (mask))
 #define has_bits(bits, mask)	(((bits) & (mask)) == (bits))
-
+/*
+#define dp_for_each_set_bit(bit, mask) \
+	for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
+#define dp_for_each_set_bit(bit, mask) \
+	unsigned long __local_mask = mask; \
+	for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
+*/
 #define dp_for_each_set_bit(bit, mask) \
-	for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
+	for_each_set_bit((bit), &(mask), sizeof(mask) * 8)
 
 #define dp_wait_cond(__cond, __tries, __min_range, __max_range)	\
 ({							\
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
index 719a79728e24..a85c8a806334 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c
@@ -46,8 +46,9 @@  void komeda_pipeline_destroy(struct komeda_dev *mdev,
 {
 	struct komeda_component *c;
 	int i;
+	unsigned long avail_comps = pipe->avail_comps;
 
-	dp_for_each_set_bit(i, pipe->avail_comps) {
+	dp_for_each_set_bit(i, avail_comps) {
 		c = komeda_pipeline_get_component(pipe, i);
 		komeda_component_destroy(mdev, c);
 	}
@@ -247,6 +248,7 @@  static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
 {
 	struct komeda_component *c;
 	int id;
+	unsigned long avail_comps = pipe->avail_comps;
 
 	DRM_INFO("Pipeline-%d: n_layers: %d, n_scalers: %d, output: %s.\n",
 		 pipe->id, pipe->n_layers, pipe->n_scalers,
@@ -258,7 +260,7 @@  static void komeda_pipeline_dump(struct komeda_pipeline *pipe)
 		 pipe->of_output_links[1] ?
 		 pipe->of_output_links[1]->full_name : "none");
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	dp_for_each_set_bit(id, avail_comps) {
 		c = komeda_pipeline_get_component(pipe, id);
 
 		komeda_component_dump(c);
@@ -270,8 +272,9 @@  static void komeda_component_verify_inputs(struct komeda_component *c)
 	struct komeda_pipeline *pipe = c->pipeline;
 	struct komeda_component *input;
 	int id;
+	unsigned long supported_inputs = c->supported_inputs;
 
-	dp_for_each_set_bit(id, c->supported_inputs) {
+	dp_for_each_set_bit(id, supported_inputs) {
 		input = komeda_pipeline_get_component(pipe, id);
 		if (!input) {
 			c->supported_inputs &= ~(BIT(id));
@@ -302,8 +305,9 @@  static void komeda_pipeline_assemble(struct komeda_pipeline *pipe)
 	struct komeda_component *c;
 	struct komeda_layer *layer;
 	int i, id;
+	unsigned long avail_comps = pipe->avail_comps;
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	dp_for_each_set_bit(id, avail_comps) {
 		c = komeda_pipeline_get_component(pipe, id);
 		komeda_component_verify_inputs(c);
 	}
@@ -355,13 +359,15 @@  void komeda_pipeline_dump_register(struct komeda_pipeline *pipe,
 {
 	struct komeda_component *c;
 	u32 id;
+	unsigned long avail_comps;
 
 	seq_printf(sf, "\n======== Pipeline-%d ==========\n", pipe->id);
 
 	if (pipe->funcs && pipe->funcs->dump_register)
 		pipe->funcs->dump_register(pipe, sf);
 
-	dp_for_each_set_bit(id, pipe->avail_comps) {
+	avail_comps = pipe->avail_comps;
+	dp_for_each_set_bit(id, avail_comps) {
 		c = komeda_pipeline_get_component(pipe, id);
 
 		seq_printf(sf, "\n------%s------\n", c->name);
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
index e8b1e15312d8..7640dae7f4bf 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c
@@ -1232,7 +1232,8 @@  komeda_pipeline_unbound_components(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *old = priv_to_pipe_st(pipe->obj.state);
 	struct komeda_component_state *c_st;
 	struct komeda_component *c;
-	u32 disabling_comps, id;
+	u32 id;
+	unsigned long disabling_comps;
 
 	WARN_ON(!old);
 
@@ -1287,7 +1288,8 @@  bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *old;
 	struct komeda_component *c;
 	struct komeda_component_state *c_st;
-	u32 id, disabling_comps = 0;
+	u32 id;
+	unsigned long disabling_comps;
 
 	old = komeda_pipeline_get_old_state(pipe, old_state);
 
@@ -1297,7 +1299,7 @@  bool komeda_pipeline_disable(struct komeda_pipeline *pipe,
 		disabling_comps = old->active_comps &
 				  pipe->standalone_disabled_comps;
 
-	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%x.\n",
+	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, disabling_comps: 0x%lx.\n",
 			 pipe->id, old->active_comps, disabling_comps);
 
 	dp_for_each_set_bit(id, disabling_comps) {
@@ -1331,13 +1333,14 @@  void komeda_pipeline_update(struct komeda_pipeline *pipe,
 	struct komeda_pipeline_state *new = priv_to_pipe_st(pipe->obj.state);
 	struct komeda_pipeline_state *old;
 	struct komeda_component *c;
-	u32 id, changed_comps = 0;
+	u32 id;
+	unsigned long changed_comps;
 
 	old = komeda_pipeline_get_old_state(pipe, old_state);
 
 	changed_comps = new->active_comps | old->active_comps;
 
-	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%x.\n",
+	DRM_DEBUG_ATOMIC("PIPE%d: active_comps: 0x%x, changed: 0x%lx.\n",
 			 pipe->id, new->active_comps, changed_comps);
 
 	dp_for_each_set_bit(id, changed_comps) {