| Message ID | 0fca3271649736053eb9649d87e1ca01b056be40.1658394124.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 673f58f62ca6fc98979d1cf3fe89c3ff33f29b2e |
| Headers | show |
| Series | ASoC: qcom: q6dsp: Fix an off-by-one in q6adm_alloc_copp() | expand |
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
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 :) )
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
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
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);
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(-)