[PATCH] sound/isa/ib: Fix two use after free in snd_sb_qsound_build

[PATCH] sound/isa/ib: Fix two use after free in snd_sb_qsound_build

[Lv Yunlong]

In snd_sb_qsound_build, snd_ctl_add(..,p->qsound_switch...) and
snd_ctl_add(..,p->qsound_space..) are called. But the second
arguments of snd_ctl_add() could be freed via snd_ctl_add_replace()
->snd_ctl_free_one(). After the error code is returned,
snd_sb_qsound_destroy(p) is called in __error branch.

But in snd_sb_qsound_destroy(), the freed p->qsound_switch and
p->qsound_space are still used by snd_ctl_remove().

My patch set p->qsound_switch and p->qsound_space to NULL if
snd_ctl_add() failed to avoid the uaf bugs. But these codes need
to further be improved with the code style.

Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>
---
 sound/isa/sb/sb16_csp.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/isa/sb/sb16_csp.c b/sound/isa/sb/sb16_csp.c
index 8635a2b6b36b..4789345a8fdd 100644
--- a/sound/isa/sb/sb16_csp.c
+++ b/sound/isa/sb/sb16_csp.c
@@ -1045,10 +1045,14 @@ static int snd_sb_qsound_build(struct snd_sb_csp * p)
 
 	spin_lock_init(&p->q_lock);
 
-	if ((err = snd_ctl_add(card, p->qsound_switch = snd_ctl_new1(&snd_sb_qsound_switch, p))) < 0)
+	if ((err = snd_ctl_add(card, p->qsound_switch = snd_ctl_new1(&snd_sb_qsound_switch, p))) < 0) {
+		p->qsound_switch = NULL;
 		goto __error;
-	if ((err = snd_ctl_add(card, p->qsound_space = snd_ctl_new1(&snd_sb_qsound_space, p))) < 0)
+	}
+	if ((err = snd_ctl_add(card, p->qsound_space = snd_ctl_new1(&snd_sb_qsound_space, p))) < 0) {
+		p->qsound_space = NULL;
 		goto __error;
+	}
 
 	return 0;
 
-- 
2.25.1

Re: [PATCH] sound/isa/ib: Fix two use after free in snd_sb_qsound_build

[Takashi Iwai]

On Mon, 26 Apr 2021 16:55:41 +0200,
Lv Yunlong wrote:
> 
> In snd_sb_qsound_build, snd_ctl_add(..,p->qsound_switch...) and
> snd_ctl_add(..,p->qsound_space..) are called. But the second
> arguments of snd_ctl_add() could be freed via snd_ctl_add_replace()
> ->snd_ctl_free_one(). After the error code is returned,
> snd_sb_qsound_destroy(p) is called in __error branch.
> 
> But in snd_sb_qsound_destroy(), the freed p->qsound_switch and
> p->qsound_space are still used by snd_ctl_remove().
> 
> My patch set p->qsound_switch and p->qsound_space to NULL if
> snd_ctl_add() failed to avoid the uaf bugs. But these codes need
> to further be improved with the code style.
> 
> Signed-off-by: Lv Yunlong <lyl2019@mail.ustc.edu.cn>

Thanks, applied now.


Takashi