linux-kernel - Re: [PATCH v2] sound: fix uninit-value in sof_ipc4_pcm_dai_link_fixup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1611f678-edaf-4588-8455-61eed32b2baa@gmail.com>
Date: Tue, 5 Nov 2024 16:20:23 +0530
From: Suraj Sonawane <surajsonawane0215@...il.com>
To: Mark Brown <broonie@...nel.org>,
 Péter Ujfalusi <peter.ujfalusi@...ux.intel.com>
Cc: daniel.baluta@....com, kai.vehmanen@...ux.intel.com, lgirdwood@...il.com,
 linux-kernel@...r.kernel.org, linux-sound@...r.kernel.org, perex@...ex.cz,
 pierre-louis.bossart@...ux.dev, ranjani.sridharan@...ux.intel.com,
 sound-open-firmware@...a-project.org, tiwai@...e.com,
 yung-chuan.liao@...ux.intel.com
Subject: Re: [PATCH v2] sound: fix uninit-value in
 sof_ipc4_pcm_dai_link_fixup_rate

On 04/11/24 23:57, Mark Brown wrote:
> On Mon, Nov 04, 2024 at 12:52:09PM +0200, Péter Ujfalusi wrote:
>> On 03/11/2024 13:37, Suraj Sonawane wrote:
> 
>>> Fix an issue detected by the Smatch tool:
>>>
>>> sound/soc/sof/ipc4-pcm.c: sof_ipc4_pcm_dai_link_fixup_rate()
>>> error: uninitialized symbol 'be_rate'.
>>>
>>> This issue occurred because the variable 'be_rate' could remain
>>> uninitialized if num_input_formats is zero. In such cases, the
>>> loop that assigns a value to 'be_rate' would not execute,
>>> potentially leading to undefined behavior when rate->min and
>>> rate->max are set with an uninitialized 'be_rate'.
>>>
>>> To resolve this, an additional check for num_input_formats > 0
>>> was added before setting rate->min and rate->max with 'be_rate'.
>>> This ensures that 'be_rate' is assigned only when there are valid
>>> input formats, preventing any use of uninitialized data.
> 
>>> -		rate->min = be_rate;
>>> -		rate->max = rate->min;
>>> +		/* Set rate only if be_rate was assigned */
>>> +		if (num_input_formats > 0) {
> 
>> By definition the copier must have at least one input and one output
>> format, this check is going to be always true.
> 
> Static analysis of the code can't reasonably tell that, we need
> something that ensures that it doesn't detect a spuriously uninitialised
> variable here.  Possibly a
> 
> 	if (WARN_ON_ONCE(!num_input_formats))
> 		return -EINVAL;
> 
> or similar?

Thank you, Mark and Péter, for the guidance. I understand now that, 
while the copier should always have at least one input format, static 
analysis tools can’t detect this. Based on your suggestions, I’ve 
considered the following possible solutions to address the issue:

1. Add a WARN_ON_ONCE(!num_input_formats) check: This would issue a 
warning and return an error if num_input_formats is unexpectedly zero, 
ensuring we handle any edge cases explicitly.

2. Return an error if no input formats are available: Implementing the 
following check could provide immediate feedback if num_input_formats is 
zero:
     if (num_input_formats <= 0) {
         dev_err(sdev->dev, "No input formats available\n");
         return -EINVAL; // Return an error if there are no formats
     }

Would it be preferable to proceed with the 
WARN_ON_ONCE(!num_input_formats) approach, or is there a preferred 
alternative from the options above?

Thank you again