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

From: Dan Carpenter <dan.carpenter@oracle.com>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	Banajit Goswami <bgoswami@quicinc.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
	Takashi Iwai <tiwai@suse.com>,
	linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
	Banajit Goswami <bgoswami@codeaurora.org>,
	alsa-devel@alsa-project.org,
	Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Subject: Re: [PATCH] ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp()
Date: Thu, 21 Jul 2022 13:00:42 +0300	[thread overview]
Message-ID: <20220721100042.GL2338@kadam> (raw)
In-Reply-To: <0fca3271649736053eb9649d87e1ca01b056be40.1658394124.git.christophe.jaillet@wanadoo.fr>

[-- 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);
}