[bug report] ASoC: SOF: avoid a NULL dereference with unsupported widgets
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
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.
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
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/...
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.
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/...
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
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/...
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.
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/...
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
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
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
participants (3)
-
Dan Carpenter -
Guennadi Liakhovetski -
Péter Ujfalusi