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

[sashiko-bot@kernel.org]

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