find_first_zero_bit() returns MAX_COPPS_PER_PORT at max here.
So 'idx' should be tested with ">=" or the test can't match.
Fixes: 7b20b2be51e1 ("ASoC: qdsp6: q6adm: Add q6adm driver")
Signed-off-by: Christophe JAILLET <[email protected]>
---
sound/soc/qcom/qdsp6/q6adm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/qcom/qdsp6/q6adm.c b/sound/soc/qcom/qdsp6/q6adm.c
index 01f383888b62..1530e98df165 100644
--- a/sound/soc/qcom/qdsp6/q6adm.c
+++ b/sound/soc/qcom/qdsp6/q6adm.c
@@ -217,7 +217,7 @@ static struct q6copp *q6adm_alloc_copp(struct q6adm *adm, int port_idx)
idx = find_first_zero_bit(&adm->copp_bitmap[port_idx],
MAX_COPPS_PER_PORT);
- if (idx > MAX_COPPS_PER_PORT)
+ if (idx >= MAX_COPPS_PER_PORT)
return ERR_PTR(-EBUSY);
c = kzalloc(sizeof(*c), GFP_ATOMIC);
--
2.34.1
On Thu, Jul 21, 2022 at 11:02:22AM +0200, Christophe JAILLET wrote:
> find_first_zero_bit() returns MAX_COPPS_PER_PORT at max here.
> So 'idx' should be tested with ">=" or the test can't match.
>
> Fixes: 7b20b2be51e1 ("ASoC: qdsp6: q6adm: Add q6adm driver")
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
> sound/soc/qcom/qdsp6/q6adm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/qcom/qdsp6/q6adm.c b/sound/soc/qcom/qdsp6/q6adm.c
> index 01f383888b62..1530e98df165 100644
> --- a/sound/soc/qcom/qdsp6/q6adm.c
> +++ b/sound/soc/qcom/qdsp6/q6adm.c
> @@ -217,7 +217,7 @@ static struct q6copp *q6adm_alloc_copp(struct q6adm *adm, int port_idx)
> idx = find_first_zero_bit(&adm->copp_bitmap[port_idx],
> MAX_COPPS_PER_PORT);
>
> - if (idx > MAX_COPPS_PER_PORT)
> + if (idx >= MAX_COPPS_PER_PORT)
> return ERR_PTR(-EBUSY);
Harshit asked me to write a Smatch check to prevent this bug in the
future. I got his email before I got your patch. :P Attached.
sound/soc/qcom/qdsp6/q6adm.c:220 q6adm_alloc_copp() warn: impossible find_next_bit condition
I'll probably try to make this check more generic, but even the simple
find_first_zero_bit() version will probably find bugs in the future and
it was pretty simple to write.
regards,
dan carpenter
On Thu, Jul 21, 2022 at 01:00:42PM +0300, Dan Carpenter wrote:
> sound/soc/qcom/qdsp6/q6adm.c:220 q6adm_alloc_copp() warn: impossible find_next_bit condition
>
> I'll probably try to make this check more generic
Attached is my first draft generic version. There are other ways I
could have written this, but I'll test my first draft and see what that
looks like.
sound/soc/qcom/qdsp6/q6adm.c:220 q6adm_alloc_copp() warn: potential off by one check 'find_first_zero_bit()'
regards,
dan carpenter
Le 21/07/2022 à 12:00, Dan Carpenter a écrit :
> On Thu, Jul 21, 2022 at 11:02:22AM +0200, Christophe JAILLET wrote:
>> find_first_zero_bit() returns MAX_COPPS_PER_PORT at max here.
>> So 'idx' should be tested with ">=" or the test can't match.
>>
>> Fixes: 7b20b2be51e1 ("ASoC: qdsp6: q6adm: Add q6adm driver")
>> Signed-off-by: Christophe JAILLET <[email protected]>
>> ---
>> sound/soc/qcom/qdsp6/q6adm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/qcom/qdsp6/q6adm.c b/sound/soc/qcom/qdsp6/q6adm.c
>> index 01f383888b62..1530e98df165 100644
>> --- a/sound/soc/qcom/qdsp6/q6adm.c
>> +++ b/sound/soc/qcom/qdsp6/q6adm.c
>> @@ -217,7 +217,7 @@ static struct q6copp *q6adm_alloc_copp(struct q6adm *adm, int port_idx)
>> idx = find_first_zero_bit(&adm->copp_bitmap[port_idx],
>> MAX_COPPS_PER_PORT);
>>
>> - if (idx > MAX_COPPS_PER_PORT)
>> + if (idx >= MAX_COPPS_PER_PORT)
>> return ERR_PTR(-EBUSY);
>
> Harshit asked me to write a Smatch check to prevent this bug in the
> future. I got his email before I got your patch. :P Attached.
Well, well, well...
Easy to say afterwards. You got 58 mins to write it. :).
>
> sound/soc/qcom/qdsp6/q6adm.c:220 q6adm_alloc_copp() warn: impossible find_next_bit condition
>
> I'll probably try to make this check more generic, but even the simple
> find_first_zero_bit() version will probably find bugs in the future and
> it was pretty simple to write.
You could add find_last_bit(), find_next_zero_bit_le() and
find_next_bit_le().
>
> regards,
> dan carpenter
>
>
A reduced version of mine was:
@@
expression e1, e2;
statement S;
@@
(
* e1 = find_first_bit(...);
|
* e1 = find_last_bit(...);
|
[... snip ...]
)
...
if (e1 > e2)
S
(and it takes only a few seconds to scan the whole kernel :) )
On Thu, Jul 21, 2022 at 12:30:32PM +0200, Christophe JAILLET wrote:
> You could add find_last_bit(), find_next_zero_bit_le() and
> find_next_bit_le().
>
Thanks!
> >
> > regards,
> > dan carpenter
> >
> >
>
> A reduced version of mine was:
>
> @@
> expression e1, e2;
> statement S;
> @@
> (
> * e1 = find_first_bit(...);
> |
> * e1 = find_last_bit(...);
> |
> [... snip ...]
> )
> ...
> if (e1 > e2)
> S
>
>
> (and it takes only a few seconds to scan the whole kernel :) )
Nice!
I wasn't going to be before but now I have to re-write my generic
check to be even more *powerful* than before! The new check doesn't
rely on known values for the limit, but uses comparison data instead.
(Still takes overnight to run so I might end up sorely dissappointed
and defeated tomorrow morning)
regards,
dan carpenter
On Thu, Jul 21, 2022 at 01:47:31PM +0300, Dan Carpenter wrote:
> (Still takes overnight to run so I might end up sorely dissappointed
> and defeated tomorrow morning)
The generic test was pretty useless. :( Basically it was 117 false
positives. Attached.
There were thre main reasons for the false postives.
1) Smatch takes short cuts when dealing with loops.
2) Smatch doesn't understand threads so some code does.
msg.code = 0;
write_msg_and_wait_for_response(&msg);
return msg.code;
It's kind of useful to find these bugs in Smatch and I'll investigate
how to fix them. Another option would be to hack around the bugs by
just ignoring 0 and 1 returns.
if (rl_max(left_rl).value == 0 || rl_max(left_rl).value == 1)
return;
That would probably silence 90% of the false positives caused by 1 and
2.
3) A lot of code has harmless sanity checks:
size = get_size();
if (size > MAX)
return -EINVAL;
or:
size = get_size();
if (size > MAX)
size = MAX;
defeated. :(
regards,
dan carpenter
On Thu, 21 Jul 2022 11:02:22 +0200, Christophe JAILLET wrote:
> find_first_zero_bit() returns MAX_COPPS_PER_PORT at max here.
> So 'idx' should be tested with ">=" or the test can't match.
>
>
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/1] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()
commit: 673f58f62ca6fc98979d1cf3fe89c3ff33f29b2e
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark