[bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Dan Carpenter]

Hello Guennadi Liakhovetski,

This is a semi-automatic email about new static checker warnings.

The patch e3720f92e023: "ASoC: SOF: avoid a NULL dereference with 
unsupported widgets" from Mar 29, 2023, leads to the following Smatch 
complaint:

sound/soc/sof/ipc4-topology.c:2353 sof_ipc4_route_setup()
error: we previously assumed 'sink_fw_module' could be null (see line 2351)

sound/soc/sof/ipc4-topology.c:2353 sof_ipc4_route_setup()
error: we previously assumed 'src_fw_module' could be null (see line 2351)

sound/soc/sof/ipc4-topology.c
  2350	
  2351		if (!src_fw_module || !sink_fw_module) {
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
One of these is NULL.

  2352			/* The NULL module will print as "(efault)" */
  2353			dev_err(sdev->dev, "source %s or sink %s widget weren't set up properly\n",
  2354				src_fw_module->man4_module_entry.name,
  2355				sink_fw_module->man4_module_entry.name);
                                ^^^^^^^^^^^^^^
Both are dereferenced.  The comment is very puzzling.

regards,
dan carpenter

Re: [bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Péter Ujfalusi]

Hi Dan,

On 31/03/2023 09:58, Dan Carpenter wrote:
> Hello Guennadi Liakhovetski,
> 
> This is a semi-automatic email about new static checker warnings.
> 
> The patch e3720f92e023: "ASoC: SOF: avoid a NULL dereference with 
> unsupported widgets" from Mar 29, 2023, leads to the following Smatch 
> complaint:
> 
> sound/soc/sof/ipc4-topology.c:2353 sof_ipc4_route_setup()
> error: we previously assumed 'sink_fw_module' could be null (see line 2351)
> 
> sound/soc/sof/ipc4-topology.c:2353 sof_ipc4_route_setup()
> error: we previously assumed 'src_fw_module' could be null (see line 2351)
> 
> sound/soc/sof/ipc4-topology.c
>   2350	
>   2351		if (!src_fw_module || !sink_fw_module) {
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> One of these is NULL.
> 
>   2352			/* The NULL module will print as "(efault)" */
>   2353			dev_err(sdev->dev, "source %s or sink %s widget weren't set up properly\n",
>   2354				src_fw_module->man4_module_entry.name,
>   2355				sink_fw_module->man4_module_entry.name);
>                                 ^^^^^^^^^^^^^^
> Both are dereferenced.  The comment is very puzzling.

if src_fw_module is NULL then the print will be:
source (efault) or sink sink.module.name widget weren't set up properly

Guennadi is relying on some black magic in the printk system to handle
the printing instead of open coding.

-- 
Péter

Re: [bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Dan Carpenter]

On Fri, Mar 31, 2023 at 10:14:11AM +0300, Péter Ujfalusi wrote:
> Hi Dan,
> 
> On 31/03/2023 09:58, Dan Carpenter wrote:
> > Hello Guennadi Liakhovetski,
> > 
> > This is a semi-automatic email about new static checker warnings.
> > 
> > The patch e3720f92e023: "ASoC: SOF: avoid a NULL dereference with 
> > unsupported widgets" from Mar 29, 2023, leads to the following Smatch 
> > complaint:
> > 
> > sound/soc/sof/ipc4-topology.c:2353 sof_ipc4_route_setup()
> > error: we previously assumed 'sink_fw_module' could be null (see line 2351)
> > 
> > sound/soc/sof/ipc4-topology.c:2353 sof_ipc4_route_setup()
> > error: we previously assumed 'src_fw_module' could be null (see line 2351)
> > 
> > sound/soc/sof/ipc4-topology.c
> >   2350	
> >   2351		if (!src_fw_module || !sink_fw_module) {
> >                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > One of these is NULL.
> > 
> >   2352			/* The NULL module will print as "(efault)" */
> >   2353			dev_err(sdev->dev, "source %s or sink %s widget weren't set up properly\n",
> >   2354				src_fw_module->man4_module_entry.name,
> >   2355				sink_fw_module->man4_module_entry.name);
> >                                 ^^^^^^^^^^^^^^
> > Both are dereferenced.  The comment is very puzzling.
> 
> if src_fw_module is NULL then the print will be:
> source (efault) or sink sink.module.name widget weren't set up properly
> 
> Guennadi is relying on some black magic in the printk system to handle
> the printing instead of open coding.

I've done compiler related work and explored some weird aspect of the
C language and I am so fascinated by this.  I would have thought it
crashes before the function is called.  I cannot even imagine how black
magic like this would work.

Is there anyway I can test this?

regards,
dan carpenter

Re: [bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Péter Ujfalusi]

On 01/04/2023 10:44, Dan Carpenter wrote:
> On Fri, Mar 31, 2023 at 10:14:11AM +0300, Péter Ujfalusi wrote:
>> if src_fw_module is NULL then the print will be:
>> source (efault) or sink sink.module.name widget weren't set up properly
>>
>> Guennadi is relying on some black magic in the printk system to handle
>> the printing instead of open coding.
> 
> I've done compiler related work and explored some weird aspect of the
> C language and I am so fascinated by this.  I would have thought it
> crashes before the function is called.  I cannot even imagine how black
> magic like this would work.

I think it is not a compiler magic, but kernel magic:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/vsprintf.c#n700

> Is there anyway I can test this?

You could, If you have a laptop which uses SOF and it is Intel 11th gen
or newer then you can switch it to IPC4 and install the opt-in v2.5
(which would need with 6.4 kernel).
Apply this patch to 6.3-rc (or 6.2) and boot up, but unpatched kernel
will NULL dereference, so you need to have a backup option.

https://github.com/thesofproject/sof-bin

The v2.5 is not there as a release,you need to fetch the repo and follow
the instructions.

Read the instruction in v2.5.x/README.md before attempting to use this
release.

Now that I look back at the patch, yes it is not obvious, but it is
doing a valid thing.

-- 
Péter

Re: [bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Dan Carpenter]

On Mon, Apr 03, 2023 at 08:20:38AM +0300, Péter Ujfalusi wrote:
> 
> 
> On 01/04/2023 10:44, Dan Carpenter wrote:
> > On Fri, Mar 31, 2023 at 10:14:11AM +0300, Péter Ujfalusi wrote:
> >> if src_fw_module is NULL then the print will be:
> >> source (efault) or sink sink.module.name widget weren't set up properly
> >>
> >> Guennadi is relying on some black magic in the printk system to handle
> >> the printing instead of open coding.
> > 
> > I've done compiler related work and explored some weird aspect of the
> > C language and I am so fascinated by this.  I would have thought it
> > crashes before the function is called.  I cannot even imagine how black
> > magic like this would work.
> 
> I think it is not a compiler magic, but kernel magic:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/vsprintf.c#n700
> 
> > Is there anyway I can test this?
> 
> You could, If you have a laptop which uses SOF and it is Intel 11th gen
> or newer then you can switch it to IPC4 and install the opt-in v2.5
> (which would need with 6.4 kernel).
> Apply this patch to 6.3-rc (or 6.2) and boot up, but unpatched kernel
> will NULL dereference, so you need to have a backup option.
> 
> https://github.com/thesofproject/sof-bin
> 
> The v2.5 is not there as a release,you need to fetch the repo and follow
> the instructions.
> 
> Read the instruction in v2.5.x/README.md before attempting to use this
> release.
> 
> Now that I look back at the patch, yes it is not obvious, but it is
> doing a valid thing.

Yeah.  Fine.  It doesn't crash but "valid" is kind of debatable.  It's
a super ugly thing.

regards,
dan carpenter

Re: [bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Péter Ujfalusi]

On 03/04/2023 08:54, Dan Carpenter wrote:
> On Mon, Apr 03, 2023 at 08:20:38AM +0300, Péter Ujfalusi wrote:
>>
>>
>> On 01/04/2023 10:44, Dan Carpenter wrote:
>>> On Fri, Mar 31, 2023 at 10:14:11AM +0300, Péter Ujfalusi wrote:
>>>> if src_fw_module is NULL then the print will be:
>>>> source (efault) or sink sink.module.name widget weren't set up properly
>>>>
>>>> Guennadi is relying on some black magic in the printk system to handle
>>>> the printing instead of open coding.
>>>
>>> I've done compiler related work and explored some weird aspect of the
>>> C language and I am so fascinated by this.  I would have thought it
>>> crashes before the function is called.  I cannot even imagine how black
>>> magic like this would work.
>>
>> I think it is not a compiler magic, but kernel magic:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/vsprintf.c#n700
>>
>>> Is there anyway I can test this?
>>
>> You could, If you have a laptop which uses SOF and it is Intel 11th gen
>> or newer then you can switch it to IPC4 and install the opt-in v2.5
>> (which would need with 6.4 kernel).
>> Apply this patch to 6.3-rc (or 6.2) and boot up, but unpatched kernel
>> will NULL dereference, so you need to have a backup option.
>>
>> https://github.com/thesofproject/sof-bin
>>
>> The v2.5 is not there as a release,you need to fetch the repo and follow
>> the instructions.
>>
>> Read the instruction in v2.5.x/README.md before attempting to use this
>> release.
>>
>> Now that I look back at the patch, yes it is not obvious, but it is
>> doing a valid thing.
> 
> Yeah.  Fine.  It doesn't crash but "valid" is kind of debatable.  It's
> a super ugly thing.

Well, it is going to behave in an expected way (print (efault) instead
of the module name in case it is not present).
By itself it is correct as this is a feature given by the kernel, on the
other hand, it could be improved to print the _swidget->widget->name and
a note, which side is missing the fw_module.

I can send a v2 with a new print without waiting for Guennadi in few hours.

-- 
Péter

Re: [bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Guennadi Liakhovetski]

Hi,

Actually there's no black magic there because there's no dereference but 
only pointer arithmetic. The compiler just adds the calculated offset to 
NULL and passes that small pointer to printk().

Thanks
Guennadi

On Mon, 3 Apr 2023, Péter Ujfalusi wrote:

>
>
> On 03/04/2023 08:54, Dan Carpenter wrote:
>> On Mon, Apr 03, 2023 at 08:20:38AM +0300, Péter Ujfalusi wrote:
>>>
>>>
>>> On 01/04/2023 10:44, Dan Carpenter wrote:
>>>> On Fri, Mar 31, 2023 at 10:14:11AM +0300, Péter Ujfalusi wrote:
>>>>> if src_fw_module is NULL then the print will be:
>>>>> source (efault) or sink sink.module.name widget weren't set up properly
>>>>>
>>>>> Guennadi is relying on some black magic in the printk system to handle
>>>>> the printing instead of open coding.
>>>>
>>>> I've done compiler related work and explored some weird aspect of the
>>>> C language and I am so fascinated by this.  I would have thought it
>>>> crashes before the function is called.  I cannot even imagine how black
>>>> magic like this would work.
>>>
>>> I think it is not a compiler magic, but kernel magic:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/vsprintf.c#n700
>>>
>>>> Is there anyway I can test this?
>>>
>>> You could, If you have a laptop which uses SOF and it is Intel 11th gen
>>> or newer then you can switch it to IPC4 and install the opt-in v2.5
>>> (which would need with 6.4 kernel).
>>> Apply this patch to 6.3-rc (or 6.2) and boot up, but unpatched kernel
>>> will NULL dereference, so you need to have a backup option.
>>>
>>> https://github.com/thesofproject/sof-bin
>>>
>>> The v2.5 is not there as a release,you need to fetch the repo and follow
>>> the instructions.
>>>
>>> Read the instruction in v2.5.x/README.md before attempting to use this
>>> release.
>>>
>>> Now that I look back at the patch, yes it is not obvious, but it is
>>> doing a valid thing.
>>
>> Yeah.  Fine.  It doesn't crash but "valid" is kind of debatable.  It's
>> a super ugly thing.
>
> Well, it is going to behave in an expected way (print (efault) instead
> of the module name in case it is not present).
> By itself it is correct as this is a feature given by the kernel, on the
> other hand, it could be improved to print the _swidget->widget->name and
> a note, which side is missing the fw_module.
>
> I can send a v2 with a new print without waiting for Guennadi in few hours.
>
> -- 
> Péter
>

Re: [bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Dan Carpenter]

On Tue, Apr 11, 2023 at 06:22:20PM +0200, Guennadi Liakhovetski wrote:
> Hi,
> 
> Actually there's no black magic there because there's no dereference but
> only pointer arithmetic. The compiler just adds the calculated offset to
> NULL and passes that small pointer to printk().
> 

The code in check_pointer_msg() is supposed to detect bugs but we're
taking advantage of it and introducing bugs...  It's might not be magic
but it's pretty astonishing.

regards,
dan carpenter

Re: [bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets

[Dan Carpenter]

On Thu, Apr 13, 2023 at 07:41:06AM +0300, Dan Carpenter wrote:
> On Tue, Apr 11, 2023 at 06:22:20PM +0200, Guennadi Liakhovetski wrote:
> > Hi,
> > 
> > Actually there's no black magic there because there's no dereference but
> > only pointer arithmetic. The compiler just adds the calculated offset to
> > NULL and passes that small pointer to printk().
> > 
> 
> The code in check_pointer_msg() is supposed to detect bugs but we're
> taking advantage of it and introducing bugs...  It's might not be magic
> but it's pretty astonishing.

"efault" is not a mispelling of default.  The "e" means error.

regards,
dan carpenter