From: Carsten Haitzler carsten.haitzler@arm.com
Another issue found by KASAN. The bit finding is bueried 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 --- .../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
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..f7dddb9f015d 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);
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe, st = komeda_pipeline_get_new_state(pipe, drm_st); else st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL); - if (WARN_ON(IS_ERR_OR_NULL(st))) return -EINVAL;
@@ -1287,7 +1287,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 +1298,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 +1332,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) {
On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
From: Carsten Haitzler carsten.haitzler@arm.com
Another issue found by KASAN. The bit finding is bueried inside the
NIT: s/bueried/buried/
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.
This seems like an underlying bug/lack of clear documentation for the underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the right thing:
#define dp_for_each_set_bit(bit, mask) \ for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
i.e. passing the actual size of type. However because of the case to unsigned long (and subsequent accesses using that type) the compiler is free to make accesses beyond the size of the variable (and indeed this is completely broken on big-endian). The implementation actually requires that the bitmap is really an array of unsigned long - no other type will work.
So I think the fix should also include fixing the dp_for_each_set_bit() macro - the cast is bogus as it stands.
Doing that I also get warnings on komeda_pipeline::avail_comps and komeda_pipeline::supported_inputs - although I note there are other bitmasks mentioned.
The other option is to fix dp_for_each_set_bit() itself using a little hack:
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8) + for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask) * 8)
With that I don't think you need any other change as the mask is actually in a new unsigned long on the stack.
Steve
Signed-off-by: Carsten Haitzler carsten.haitzler@arm.com
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
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..f7dddb9f015d 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);
@@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe, st = komeda_pipeline_get_new_state(pipe, drm_st); else st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
NIT: Random white space change
if (WARN_ON(IS_ERR_OR_NULL(st))) return -EINVAL;
@@ -1287,7 +1287,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 +1298,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 +1332,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) {
On 1/20/21 3:44 PM, Steven Price wrote:
On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
From: Carsten Haitzler carsten.haitzler@arm.com
Another issue found by KASAN. The bit finding is bueried inside the
NIT: s/bueried/buried/
Yup. typo not spotted by me. Thanks. Also - i spotted an accidental whitespace change along the way so can fix both in a new patch.
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.
This seems like an underlying bug/lack of clear documentation for the underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the right thing:
Correct. This was a general problem I spotted - the bit funcs were written to want a unsigned long but were being used on u32's by just casting the ptr to the type and this did indeed have technical issues.
#define dp_for_each_set_bit(bit, mask) \ for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask)
i.e. passing the actual size of type. However because of the case to unsigned long (and subsequent accesses using that type) the compiler is free to make accesses beyond the size of the variable (and indeed this is completely broken on big-endian). The implementation actually requires that the bitmap is really an array of unsigned long - no other type will work.
Precisely. So a bit loose. The bit funcs are used widely enough, so just fixing this code here to pass in the expected type is probably the least disruptive fix.
So I think the fix should also include fixing the dp_for_each_set_bit() macro - the cast is bogus as it stands.
Yup. Removing the cast does indeed find more badness that needs fixing. I'll do an updated patch with this.
Doing that I also get warnings on komeda_pipeline::avail_comps and komeda_pipeline::supported_inputs - although I note there are other bitmasks mentioned.
The other option is to fix dp_for_each_set_bit() itself using a little hack:
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask)
+ for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask)
With that I don't think you need any other change as the mask is actually in a new unsigned long on the stack.
That would be wonderful if it worked :). Unfortunately your above proposal leads to:
./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: error: lvalue required as unary ‘&’ operand 17 | for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8) | ^ ./include/linux/bitops.h:34:30: note: in definition of macro ‘for_each_set_bit’ 34 | (bit) = find_next_bit((addr), (size), (bit) + 1)) | ^~~~ drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: note: in expansion of macro ‘dp_for_each_set_bit’ 1243 | dp_for_each_set_bit(id, disabling_comps) { | ^~~~~~~~~~~~~~~~~~~
Basically can't take address of an "unnamed local var". :| That is with:
#define dp_for_each_set_bit(bit, mask) \ for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
I could try have the dp_for_each macro create new local vars on its own a bit like:
#define dp_for_each_set_bit(bit, mask) \ unsigned long __local_mask = mask; \ for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
But we all know where this leads... (multiple bad places with adding warnings and potential and pitfalls that then lead with ever more invasive changes to address like if code in future might do if (x) dp_for_each...). I'd prefer to be able to write code more loosely (pass in any time and it just does the right thing), but trying to balance this with least disruption and ugliness.
Steve
Signed-off-by: Carsten Haitzler carsten.haitzler@arm.com
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
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..f7dddb9f015d 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); @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe, st = komeda_pipeline_get_new_state(pipe, drm_st); else st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
NIT: Random white space change
if (WARN_ON(IS_ERR_OR_NULL(st))) return -EINVAL; @@ -1287,7 +1287,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 +1298,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 +1332,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) {
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 21/01/2021 12:22, Carsten Haitzler wrote:
On 1/20/21 3:44 PM, Steven Price wrote:
On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
From: Carsten Haitzler carsten.haitzler@arm.com
Another issue found by KASAN. The bit finding is bueried inside the
NIT: s/bueried/buried/
Yup. typo not spotted by me. Thanks. Also - i spotted an accidental whitespace change along the way so can fix both in a new patch.
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.
This seems like an underlying bug/lack of clear documentation for the underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the right thing:
Correct. This was a general problem I spotted - the bit funcs were written to want a unsigned long but were being used on u32's by just casting the ptr to the type and this did indeed have technical issues.
#define dp_for_each_set_bit(bit, mask) \ for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
i.e. passing the actual size of type. However because of the case to unsigned long (and subsequent accesses using that type) the compiler is free to make accesses beyond the size of the variable (and indeed this is completely broken on big-endian). The implementation actually requires that the bitmap is really an array of unsigned long - no other type will work.
Precisely. So a bit loose. The bit funcs are used widely enough, so just fixing this code here to pass in the expected type is probably the least disruptive fix.
So I think the fix should also include fixing the dp_for_each_set_bit() macro - the cast is bogus as it stands.
Yup. Removing the cast does indeed find more badness that needs fixing. I'll do an updated patch with this.
Doing that I also get warnings on komeda_pipeline::avail_comps and komeda_pipeline::supported_inputs - although I note there are other bitmasks mentioned.
The other option is to fix dp_for_each_set_bit() itself using a little hack:
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8) + for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask) * 8)
With that I don't think you need any other change as the mask is actually in a new unsigned long on the stack.
That would be wonderful if it worked :). Unfortunately your above proposal leads to:
./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: error: lvalue required as unary ‘&’ operand 17 | for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8) | ^
Looks like you didn't notice the subtle change above. My change uses braces ('{}') around 'mask' - I believe it's a GCC extension ("compound literals") and it creates an lvalue so you can then take the address of it...
I'm not sure if it's a good approach to the problem or not. The alternative is to fix up various places to use unsigned longs so you can use the unwrapped for_each_set_bit().
Steve
./include/linux/bitops.h:34:30: note: in definition of macro ‘for_each_set_bit’ 34 | (bit) = find_next_bit((addr), (size), (bit) + 1)) | ^~~~ drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: note: in expansion of macro ‘dp_for_each_set_bit’ 1243 | dp_for_each_set_bit(id, disabling_comps) { | ^~~~~~~~~~~~~~~~~~~
Basically can't take address of an "unnamed local var". :| That is with:
#define dp_for_each_set_bit(bit, mask) \ for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
I could try have the dp_for_each macro create new local vars on its own a bit like:
#define dp_for_each_set_bit(bit, mask) \ unsigned long __local_mask = mask; \ for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
But we all know where this leads... (multiple bad places with adding warnings and potential and pitfalls that then lead with ever more invasive changes to address like if code in future might do if (x) dp_for_each...). I'd prefer to be able to write code more loosely (pass in any time and it just does the right thing), but trying to balance this with least disruption and ugliness.
Steve
Signed-off-by: Carsten Haitzler carsten.haitzler@arm.com
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
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..f7dddb9f015d 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); @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe, st = komeda_pipeline_get_new_state(pipe, drm_st); else st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
NIT: Random white space change
if (WARN_ON(IS_ERR_OR_NULL(st))) return -EINVAL; @@ -1287,7 +1287,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 +1298,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 +1332,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) {
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 1/21/21 4:40 PM, Steven Price wrote:
On 21/01/2021 12:22, Carsten Haitzler wrote:
On 1/20/21 3:44 PM, Steven Price wrote:
On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
From: Carsten Haitzler carsten.haitzler@arm.com
Another issue found by KASAN. The bit finding is bueried inside the
NIT: s/bueried/buried/
Yup. typo not spotted by me. Thanks. Also - i spotted an accidental whitespace change along the way so can fix both in a new patch.
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.
This seems like an underlying bug/lack of clear documentation for the underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the right thing:
Correct. This was a general problem I spotted - the bit funcs were written to want a unsigned long but were being used on u32's by just casting the ptr to the type and this did indeed have technical issues.
#define dp_for_each_set_bit(bit, mask) \ for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8)
i.e. passing the actual size of type. However because of the case to unsigned long (and subsequent accesses using that type) the compiler is free to make accesses beyond the size of the variable (and indeed this is completely broken on big-endian). The implementation actually requires that the bitmap is really an array of unsigned long - no other type will work.
Precisely. So a bit loose. The bit funcs are used widely enough, so just fixing this code here to pass in the expected type is probably the least disruptive fix.
So I think the fix should also include fixing the dp_for_each_set_bit() macro - the cast is bogus as it stands.
Yup. Removing the cast does indeed find more badness that needs fixing. I'll do an updated patch with this.
Doing that I also get warnings on komeda_pipeline::avail_comps and komeda_pipeline::supported_inputs - although I note there are other bitmasks mentioned.
The other option is to fix dp_for_each_set_bit() itself using a little hack:
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask) * 8) + for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask) * 8)
With that I don't think you need any other change as the mask is actually in a new unsigned long on the stack.
That would be wonderful if it worked :). Unfortunately your above proposal leads to:
./drivers/gpu/drm/arm/display/komeda/../include/malidp_utils.h:17:27: error: lvalue required as unary ‘&’ operand 17 | for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8) | ^
Looks like you didn't notice the subtle change above. My change uses braces ('{}') around 'mask' - I believe it's a GCC extension ("compound literals") and it creates an lvalue so you can then take the address of it...
Oh... ewww. I did indeed skip the {}'s and just looked at them as ()'s :) I'm not so hot on using such extensions if it can be avoided. :) I just removed the cast and fixed up all the usages. Patch will come with this.
I'm not sure if it's a good approach to the problem or not. The alternative is to fix up various places to use unsigned longs so you can use the unwrapped for_each_set_bit().
Steve
./include/linux/bitops.h:34:30: note: in definition of macro ‘for_each_set_bit’ 34 | (bit) = find_next_bit((addr), (size), (bit) + 1)) | ^~~~ drivers/gpu/drm/arm/display/komeda/komeda_pipeline_state.c:1243:2: note: in expansion of macro ‘dp_for_each_set_bit’ 1243 | dp_for_each_set_bit(id, disabling_comps) { | ^~~~~~~~~~~~~~~~~~~
Basically can't take address of an "unnamed local var". :| That is with:
#define dp_for_each_set_bit(bit, mask) \ for_each_set_bit((bit), (&((unsigned long)(mask))), sizeof(mask) * 8)
I could try have the dp_for_each macro create new local vars on its own a bit like:
#define dp_for_each_set_bit(bit, mask) \ unsigned long __local_mask = mask; \ for_each_set_bit((bit), (&__local_mask), sizeof(mask) * 8)
But we all know where this leads... (multiple bad places with adding warnings and potential and pitfalls that then lead with ever more invasive changes to address like if code in future might do if (x) dp_for_each...). I'd prefer to be able to write code more loosely (pass in any time and it just does the right thing), but trying to balance this with least disruption and ugliness.
Steve
Signed-off-by: Carsten Haitzler carsten.haitzler@arm.com
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
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..f7dddb9f015d 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); @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe, st = komeda_pipeline_get_new_state(pipe, drm_st); else st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
NIT: Random white space change
if (WARN_ON(IS_ERR_OR_NULL(st))) return -EINVAL; @@ -1287,7 +1287,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 +1298,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 +1332,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) {
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 1/20/21 3:44 PM, Steven Price wrote:
Sent a new patch to list with updates as discussed.
On 18/01/2021 14:20, carsten.haitzler@foss.arm.com wrote:
From: Carsten Haitzler carsten.haitzler@arm.com
Another issue found by KASAN. The bit finding is bueried inside the
NIT: s/bueried/buried/
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.
This seems like an underlying bug/lack of clear documentation for the underlying find_*_bit() functions. dp_for_each_set_bit() tries to do the right thing:
#define dp_for_each_set_bit(bit, mask) \ for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask)
i.e. passing the actual size of type. However because of the case to unsigned long (and subsequent accesses using that type) the compiler is free to make accesses beyond the size of the variable (and indeed this is completely broken on big-endian). The implementation actually requires that the bitmap is really an array of unsigned long - no other type will work.
So I think the fix should also include fixing the dp_for_each_set_bit() macro - the cast is bogus as it stands.
Doing that I also get warnings on komeda_pipeline::avail_comps and komeda_pipeline::supported_inputs - although I note there are other bitmasks mentioned.
The other option is to fix dp_for_each_set_bit() itself using a little hack:
- for_each_set_bit((bit), ((unsigned long *)&(mask)), sizeof(mask)
+ for_each_set_bit((bit), (&((unsigned long){mask})), sizeof(mask)
With that I don't think you need any other change as the mask is actually in a new unsigned long on the stack.
Steve
Signed-off-by: Carsten Haitzler carsten.haitzler@arm.com
.../drm/arm/display/komeda/komeda_pipeline_state.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
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..f7dddb9f015d 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); @@ -1262,7 +1263,6 @@ int komeda_release_unclaimed_resources(struct komeda_pipeline *pipe, st = komeda_pipeline_get_new_state(pipe, drm_st); else st = komeda_pipeline_get_state_and_set_crtc(pipe, drm_st, NULL);
NIT: Random white space change
if (WARN_ON(IS_ERR_OR_NULL(st))) return -EINVAL; @@ -1287,7 +1287,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 +1298,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 +1332,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) {
dri-devel@lists.freedesktop.org
-
Carsten Haitzler -
carsten.haitzler＠foss.arm.com
-
Steven Price