| Message ID | 20210204131102.68658-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 |
+CC the other Komeda maintainers On 04/02/2021 13:11, 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> Looks fine to me: Reviewed-by: Steven Price <steven.price@arm.com> > --- > .../drm/arm/display/include/malidp_utils.h | 3 --- > .../drm/arm/display/komeda/komeda_pipeline.c | 16 +++++++++++----- > .../display/komeda/komeda_pipeline_state.c | 19 +++++++++++-------- > 3 files changed, 22 insertions(+), 16 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..49a1d7f3539c 100644 > --- a/drivers/gpu/drm/arm/display/include/malidp_utils.h > +++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h > @@ -13,9 +13,6 @@ > #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_wait_cond(__cond, __tries, __min_range, __max_range) \ > ({ \ > int num_tries = __tries; \ > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c > index 719a79728e24..06c595378dda 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) { > + for_each_set_bit(i, &avail_comps, 32) { > 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) { > + for_each_set_bit(id, &avail_comps, 32) { > 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) { > + for_each_set_bit(id, &supported_inputs, 32) { > 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) { > + for_each_set_bit(id, &avail_comps, 32) { > 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; > + for_each_set_bit(id, &avail_comps, 32) { > 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..176cdf411f9f 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c > @@ -1232,14 +1232,15 @@ 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); > > disabling_comps = (~new->active_comps) & old->active_comps; > > /* unbound all disabling component */ > - dp_for_each_set_bit(id, disabling_comps) { > + for_each_set_bit(id, &disabling_comps, 32) { > c = komeda_pipeline_get_component(pipe, id); > c_st = komeda_component_get_state_and_set_user(c, > drm_st, NULL, new->crtc); > @@ -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,10 +1299,10 @@ 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) { > + for_each_set_bit(id, &disabling_comps, 32) { > c = komeda_pipeline_get_component(pipe, id); > c_st = priv_to_comp_st(c->obj.state); > > @@ -1331,16 +1333,17 @@ 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) { > + for_each_set_bit(id, &changed_comps, 32) { > c = komeda_pipeline_get_component(pipe, id); > > if (new->active_comps & BIT(c->id)) >
On Fri, Feb 05, 2021 at 08:29:32AM +0000, Steven Price wrote: > +CC the other Komeda maintainers > > On 04/02/2021 13:11, 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> > > Looks fine to me: > > Reviewed-by: Steven Price <steven.price@arm.com> > Thank you for the patch. Reviewed-by: James Qian Wang <james.qian.wang@arm.com> > > --- > > .../drm/arm/display/include/malidp_utils.h | 3 --- > > .../drm/arm/display/komeda/komeda_pipeline.c | 16 +++++++++++----- > > .../display/komeda/komeda_pipeline_state.c | 19 +++++++++++-------- > > 3 files changed, 22 insertions(+), 16 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..49a1d7f3539c 100644 > > --- a/drivers/gpu/drm/arm/display/include/malidp_utils.h > > +++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h > > @@ -13,9 +13,6 @@ > > #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_wait_cond(__cond, __tries, __min_range, __max_range) \ > > ({ \ > > int num_tries = __tries; \ > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c > > index 719a79728e24..06c595378dda 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) { > > + for_each_set_bit(i, &avail_comps, 32) { > > 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) { > > + for_each_set_bit(id, &avail_comps, 32) { > > 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) { > > + for_each_set_bit(id, &supported_inputs, 32) { > > 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) { > > + for_each_set_bit(id, &avail_comps, 32) { > > 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; > > + for_each_set_bit(id, &avail_comps, 32) { > > 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..176cdf411f9f 100644 > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c > > @@ -1232,14 +1232,15 @@ 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); > > disabling_comps = (~new->active_comps) & old->active_comps; > > /* unbound all disabling component */ > > - dp_for_each_set_bit(id, disabling_comps) { > > + for_each_set_bit(id, &disabling_comps, 32) { > > c = komeda_pipeline_get_component(pipe, id); > > c_st = komeda_component_get_state_and_set_user(c, > > drm_st, NULL, new->crtc); > > @@ -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,10 +1299,10 @@ 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) { > > + for_each_set_bit(id, &disabling_comps, 32) { > > c = komeda_pipeline_get_component(pipe, id); > > c_st = priv_to_comp_st(c->obj.state); > > @@ -1331,16 +1333,17 @@ 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) { > > + for_each_set_bit(id, &changed_comps, 32) { > > c = komeda_pipeline_get_component(pipe, id); > > if (new->active_comps & BIT(c->id)) > >
diff --git a/drivers/gpu/drm/arm/display/include/malidp_utils.h b/drivers/gpu/drm/arm/display/include/malidp_utils.h index 3bc383d5bf73..49a1d7f3539c 100644 --- a/drivers/gpu/drm/arm/display/include/malidp_utils.h +++ b/drivers/gpu/drm/arm/display/include/malidp_utils.h @@ -13,9 +13,6 @@ #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_wait_cond(__cond, __tries, __min_range, __max_range) \ ({ \ int num_tries = __tries; \ diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c index 719a79728e24..06c595378dda 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) { + for_each_set_bit(i, &avail_comps, 32) { 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) { + for_each_set_bit(id, &avail_comps, 32) { 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) { + for_each_set_bit(id, &supported_inputs, 32) { 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) { + for_each_set_bit(id, &avail_comps, 32) { 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; + for_each_set_bit(id, &avail_comps, 32) { 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..176cdf411f9f 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c @@ -1232,14 +1232,15 @@ 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); disabling_comps = (~new->active_comps) & old->active_comps; /* unbound all disabling component */ - dp_for_each_set_bit(id, disabling_comps) { + for_each_set_bit(id, &disabling_comps, 32) { c = komeda_pipeline_get_component(pipe, id); c_st = komeda_component_get_state_and_set_user(c, drm_st, NULL, new->crtc); @@ -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,10 +1299,10 @@ 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) { + for_each_set_bit(id, &disabling_comps, 32) { c = komeda_pipeline_get_component(pipe, id); c_st = priv_to_comp_st(c->obj.state); @@ -1331,16 +1333,17 @@ 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) { + for_each_set_bit(id, &changed_comps, 32) { c = komeda_pipeline_get_component(pipe, id); if (new->active_comps & BIT(c->id))