[PATCH] ALSA: cs46xx: Handle SCB creation failures before dereferencing

[PATCH] ALSA: cs46xx: Handle SCB creation failures before dereferencing

[Alex Kazinsky]

With CONFIG_PM_SLEEP enabled, cs46xx_dsp_create_scb() can return NULL
if kmemdup() fails to allocate the SCB data saved for resume.
cs46xx_dsp_scb_and_task_init() and _dsp_create_generic_scb() dereference
the returned pointer without checking it, causing a NULL pointer
dereference on allocation failure.

Check both return values before accessing the SCB descriptors.
Return -ENOMEM when creating the null SCB fails and propagate NULL
from _dsp_create_generic_scb().

Found by Linux Verification Center (linuxtesting.org) with SVACE
static analysis tool.

Fixes: 41116e926cb9 ("ALSA: cs46xx - Fix suspend/resume with new DSP")
Cc: stable@vger.kernel.org
Signed-off-by: Alex Kazinsky <a.kazinsky@ideco.ru>
---
 sound/pci/cs46xx/dsp_spos.c         | 2 ++
 sound/pci/cs46xx/dsp_spos_scb_lib.c | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/pci/cs46xx/dsp_spos.c b/sound/pci/cs46xx/dsp_spos.c
index a77e6f5a14ad..57b0a6321e0b 100644
--- a/sound/pci/cs46xx/dsp_spos.c
+++ b/sound/pci/cs46xx/dsp_spos.c
@@ -1104,6 +1104,8 @@ int cs46xx_dsp_scb_and_task_init (struct snd_cs46xx *chip)
 
 		null_scb.entry_point = null_algorithm->address;
 		ins->the_null_scb = cs46xx_dsp_create_scb(chip, "nullSCB", (u32 *)&null_scb, NULL_SCB_ADDR);
+		if (!ins->the_null_scb)
+			return -ENOMEM;
 		ins->the_null_scb->task_entry = null_algorithm;
 		ins->the_null_scb->sub_list_ptr = ins->the_null_scb;
 		ins->the_null_scb->next_scb_ptr = ins->the_null_scb;
diff --git a/sound/pci/cs46xx/dsp_spos_scb_lib.c b/sound/pci/cs46xx/dsp_spos_scb_lib.c
index 71eee5cb478f..ac914bf784c8 100644
--- a/sound/pci/cs46xx/dsp_spos_scb_lib.c
+++ b/sound/pci/cs46xx/dsp_spos_scb_lib.c
@@ -274,7 +274,8 @@ _dsp_create_generic_scb (struct snd_cs46xx *chip, char * name, u32 * scb_data, u
 	dev_dbg(chip->card->dev, "dsp_spos: creating SCB <%s>\n", name);
 
 	scb = cs46xx_dsp_create_scb(chip,name,scb_data,dest);
-
+	if (!scb)
+		return NULL;
 
 	scb->sub_list_ptr = ins->the_null_scb;
 	scb->next_scb_ptr = ins->the_null_scb;
-- 
2.56.0

Re: [PATCH] ALSA: cs46xx: Handle SCB creation failures before dereferencing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: cs46xx: NULL deref in cs46xx_dsp_async_init() if timing_master_scb fails
--

commit 48c9250fe2fa75eef791b48e6968fdc6c7b7ecda
Author: Alex Kazinsky <a.kazinsky@ideco.ru>

ALSA: cs46xx: Handle SCB creation failures before dereferencing

This commit adds NULL checks for the return value of cs46xx_dsp_create_scb()
in cs46xx_dsp_scb_and_task_init() and _dsp_create_generic_scb() to prevent
NULL pointer dereferences when kmemdup() fails under CONFIG_PM_SLEEP.

> diff --git a/sound/pci/cs46xx/dsp_spos_scb_lib.c b/sound/pci/cs46xx/dsp_spos_scb_lib.c
> index 71eee5cb478f4..ac914bf784c81 100644
> --- a/sound/pci/cs46xx/dsp_spos_scb_lib.c
> +++ b/sound/pci/cs46xx/dsp_spos_scb_lib.c
> @@ -274,7 +274,8 @@ _dsp_create_generic_scb (struct snd_cs46xx *chip, char * name, u32 * scb_data, u
>  	dev_dbg(chip->card->dev, "dsp_spos: creating SCB <%s>\n", name);
>  
>  	scb = cs46xx_dsp_create_scb(chip,name,scb_data,dest);
> -
> +	if (!scb)
> +		return NULL;
>  
>  	scb->sub_list_ptr = ins->the_null_scb;
>  	scb->next_scb_ptr = ins->the_null_scb;

[Severity: High]
Now that _dsp_create_generic_scb() returns NULL when cs46xx_dsp_create_scb()
fails, can this still lead to a NULL pointer dereference in
cs46xx_dsp_async_init() if cs46xx_dsp_create_timing_master_scb() fails?

In sound/pci/cs46xx/dsp_spos.c:cs46xx_dsp_scb_and_task_init(),
timing_master_scb is created via:

	cs46xx_dsp_create_timing_master_scb()
	  -> cs46xx_dsp_create_generic_scb()
	    -> _dsp_create_generic_scb()

Unlike the subsequent SCB creations in cs46xx_dsp_scb_and_task_init(),
timing_master_scb is not checked for NULL:

	/* create timing master SCB */
	timing_master_scb = cs46xx_dsp_create_timing_master_scb(chip);

	/* create the CODEC output task */
	codec_out_scb = cs46xx_dsp_create_codec_out_scb(chip,"CodecOutSCB_I",0x0010,0x0000,
							MASTERMIX_SCB_ADDR,
							CODECOUT_SCB_ADDR,timing_master_scb,
							SCB_ON_PARENT_SUBLIST_SCB);

If timing_master_scb is NULL, its child SCB creations (codec_out_scb and
write_back_scb) pass parent_scb == NULL into _dsp_create_generic_scb(), skip
the if (scb->parent_scb_ptr) block, and succeed if their own allocations
succeed. Execution then continues to the end of
cs46xx_dsp_scb_and_task_init():

	cs46xx_dsp_async_init(chip,timing_master_scb);
	return 0;

Inside cs46xx_dsp_async_init(), fg_entry (which is timing_master_scb) is
dereferenced unconditionally:

		/* this one is faked, as the parnet of SPDIFO task
		   is the FG task tree */
		fg_entry->parent_scb_ptr = spdifo_scb_desc;

Additionally, cs46xx_dsp_scb_and_task_init() ignores the return value of
cs46xx_dsp_async_init(), which calls cs46xx_dsp_create_scb() three times and
returns -EIO on failure, leaving any partially created SCB descriptors in
ins->scbs[] with NULL sub_list_ptr, next_scb_ptr, and task_entry pointers
that can later be dereferenced in cs46xx_dsp_proc_scb_read().

Should cs46xx_dsp_scb_and_task_init() check timing_master_scb for NULL and
check the return value of cs46xx_dsp_async_init()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030142529.415697-1-a.kazinsky@ideco.ru?part=1