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

James Qian Wang james.qian.wang at arm.com
Fri Feb 5 08:50:26 UTC 2021


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 at foss.arm.com wrote:
> > From: Carsten Haitzler <carsten.haitzler at 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 at arm.com>
> 
> Looks fine to me:
> 
> Reviewed-by: Steven Price <steven.price at arm.com>
>

Thank you for the patch.

Reviewed-by: James Qian Wang <james.qian.wang at 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))
> > 


More information about the dri-devel mailing list