[PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()

[PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()

[Christophe JAILLET]

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 <christophe.jaillet@wanadoo.fr>
---
 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

Re: [PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

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 <christophe.jaillet@wanadoo.fr>
> ---
>  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



[-- Attachment #2: check_find_next_bit_off_by_one.c --]
[-- Type: text/x-csrc, Size: 1749 bytes --]

/*
 * Copyright (C) 2022 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

STATE(next);

static void match_next_bit(struct expression *expr, const char *name, struct symbol *sym, void *data)
{
	set_state(my_id, name, sym, &next);
}

static void match_condition(struct expression *expr)
{
	sval_t sval;

	if (expr->type != EXPR_COMPARE)
		return;
	if (expr->op != '>' && expr->op != SPECIAL_UNSIGNED_GT)
		return;

	if (!get_state_expr(my_id, expr->left))
		return;


	if (!get_implied_value(expr, &sval) || sval.value != 0)
		return;

	sm_warning("impossible find_next_bit condition");
}

void check_find_next_bit_off_by_one(int id)
{
	my_id = id;

	if (option_project != PROJ_KERNEL)
		return;

	add_function_param_key_hook("find_first_bit", match_next_bit, -1, "$", NULL);
	add_function_param_key_hook("find_next_bit", match_next_bit, -1, "$", NULL);
	add_function_param_key_hook("find_next_zero_bit", match_next_bit, -1, "$", NULL);
	add_function_param_key_hook("find_first_zero_bit", match_next_bit, -1, "$", NULL);

	add_hook(&match_condition, CONDITION_HOOK);
}

Re: [PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()

[Christophe JAILLET]

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 <christophe.jaillet@wanadoo.fr>
>> ---
>>   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 :) )

Re: [PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

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



[-- Attachment #2: check_off_by_one_capped_return.c --]
[-- Type: text/x-csrc, Size: 1528 bytes --]

/*
 * Copyright (C) 2022 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

static void match_condition(struct expression *expr)
{
	struct expression *prev;
	int comparison;
	sval_t sval;
	char *name;

	if (expr->type != EXPR_COMPARE)
		return;
	if (expr->op != '>' && expr->op != SPECIAL_UNSIGNED_GT)
		return;

	if (!get_implied_value(expr, &sval) || sval.value != 0)
		return;

	comparison = get_comparison(expr->left, expr->right);
	if (!comparison)
		return;
	if (show_special(comparison)[1] != '=')
		return;

	prev = get_assigned_expr(expr->left);
	prev = strip_expr(prev);
	if (!prev || prev->type != EXPR_CALL)
		return;

	name = expr_to_str(prev->fn);
	sm_warning("potential off by one check '%s()'", name);
	free_string(name);
}

void check_off_by_one_capped_return(int id)
{
	my_id = id;

	add_hook(&match_condition, CONDITION_HOOK);
}

Re: [PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()

[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

Re: [PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

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


[-- Attachment #2: check_off_by_one_capped_return.c --]
[-- Type: text/x-csrc, Size: 1584 bytes --]

/*
 * Copyright (C) 2022 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_extra.h"

static int my_id;

static void match_condition(struct expression *expr)
{
	struct range_list *left_rl, *right_rl;
	struct expression *prev;
	sval_t sval;
	char *name;

	if (expr->type != EXPR_COMPARE)
		return;
	if (expr->op != '>' && expr->op != SPECIAL_UNSIGNED_GT)
		return;

	if (!get_implied_value(expr, &sval) || sval.value != 0)
		return;

	if (!get_implied_rl(expr->left, &left_rl) ||
	    !get_implied_rl(expr->right, &right_rl))
		return;

	if (rl_max(left_rl).value != rl_min(right_rl).value)
		return;

	prev = get_assigned_expr(expr->left);
	prev = strip_expr(prev);
	if (!prev || prev->type != EXPR_CALL)
		return;

	name = expr_to_str(prev->fn);
	sm_warning("potential off by one check '%s()'", name);
	free_string(name);
}

void check_off_by_one_capped_return(int id)
{
	my_id = id;

	add_hook(&match_condition, CONDITION_HOOK);
}

Re: [PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()

[Mark Brown]

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