From: Steven Price <steven.price@arm.com>
To: carsten.haitzler@foss.arm.com, dri-devel@lists.freedesktop.org
Cc: "James \(Qian\) Wang" <james.qian.wang@arm.com>,
Mihail Atanassov <mihail.atanassov@arm.com>,
liviu.dudau@arm.com, Carsten Haitzler <carsten.haitzler@arm.com>
Subject: Re: [PATCH] drm/komeda: Fix bit check to import to value of proper type
Date: Fri, 5 Feb 2021 08:29:32 +0000 [thread overview]
Message-ID: <cd3022d0-d9fc-fd42-105b-5bd74346a41f@arm.com> (raw)
In-Reply-To: <20210204131102.68658-1-carsten.haitzler@foss.arm.com>
+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))
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2021-02-05 8:29 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-04 13:11 [PATCH] drm/komeda: Fix bit check to import to value of proper type carsten.haitzler
2021-02-05 8:29 ` Steven Price [this message]
2021-02-05 8:50 ` James Qian Wang
-- strict thread matches above, loose matches on Subject: below --
2021-01-27 12:34 carsten.haitzler
2021-01-27 16:31 ` Steven Price
2021-01-18 14:20 carsten.haitzler
2021-01-20 15:44 ` Steven Price
2021-01-21 12:22 ` Carsten Haitzler
2021-01-21 16:40 ` Steven Price
2021-01-21 17:37 ` Carsten Haitzler
2021-01-27 12:35 ` Carsten Haitzler
2020-12-18 15:08 carsten.haitzler
2020-12-18 16:08 ` Liviu Dudau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cd3022d0-d9fc-fd42-105b-5bd74346a41f@arm.com \
--to=steven.price@arm.com \
--cc=carsten.haitzler@arm.com \
--cc=carsten.haitzler@foss.arm.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=james.qian.wang@arm.com \
--cc=liviu.dudau@arm.com \
--cc=mihail.atanassov@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.