Re: [PATCH] ASoC: SOF: Fix null pointer dereference in sof_pci_probe

[Kunwu Chan]

It's my bad, i'll follow your suggestion in v2 patch:
1. remove 'Fixes: 46207ca24545' label

2. rename subject to 'ASoC: SOF: Add null pointer check to sof_pci_probe'

3. when sof_pdata->fw_filename_prefix is NULL,don't use it to dev_dbg. 
Or just use a
if (sof_pdata->fw_filename_prefix)
        dev_dbg(dev,
                "Platform uses community key, changed fw path to %s\n",
                sof_pdata->fw_filename_prefix);
else
	dev_dbg(dev,
                "Platform uses community key, changed fw path to %s/%s\n",
                 sof_pdata->desc->default_fw_path[sof_pdata->ipc_type],
                 "community");

Is it okay to modify it like this?


Thanks,
Kunwu

On 2023/12/4 19:34, Péter Ujfalusi wrote:
> On 23/11/2023 17:54, Kunwu Chan wrote:
> > devm_kasprintf() returns a pointer to dynamically allocated memory
> > which can be NULL upon failure.
> >
> > Fixes: 46207ca24545 ("ASoC: SOF: pci: change the default firmware path when the community key is used")
> > Fixes: 25bbc0c59ee1 ("ASoC: SOF: Add path definition for external firmware libraries")
> > Signed-off-by: Kunwu Chan <chentao@xxxxxxxxxx>
>
> This patch cannot fix both commits as at 46207ca24545 the lib path was
> not present.
> Please don't bundle unrelated fixes, it makes maintainers life harder.
>
> As a note: if the allocation fails it is not going to cause null
> dereference as the code checks !sof_pdata->*_prefix to see if it is
> provided (override) or not (use default).
> At most it will cause the kernel to 'ignore' the path override, but if
> this allocation fails this is least of our problem ;)
>
> > ---
> >   sound/soc/sof/sof-pci-dev.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/sound/soc/sof/sof-pci-dev.c b/sound/soc/sof/sof-pci-dev.c
> > index 64b326e3ef85..75a9bfa5bfbe 100644
> > --- a/sound/soc/sof/sof-pci-dev.c
> > +++ b/sound/soc/sof/sof-pci-dev.c
> > @@ -282,6 +282,10 @@ int sof_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
> >   			devm_kasprintf(dev, GFP_KERNEL, "%s/%s",
> >   				       sof_pdata->desc->default_fw_path[sof_pdata->ipc_type],
> >   				       "community");
> > +		if (!sof_pdata->fw_filename_prefix) {
> > +			ret = -ENOMEM;	
> > +			goto out;
> > +		}
> >   
> >   		dev_dbg(dev,
> >   			"Platform uses community key, changed fw path to %s\n",
> > @@ -303,6 +307,10 @@ int sof_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
> >   				devm_kasprintf(dev, GFP_KERNEL, "%s/%s",
> >   					sof_pdata->desc->default_lib_path[sof_pdata->ipc_type],
> >   					"community");
> > +			if (!sof_pdata->fw_lib_prefix) {
> > +				ret = -ENOMEM;
> > +				goto out;
> > +			}
> >   
> >   			dev_dbg(dev,
> >   				"Platform uses community key, changed fw_lib path to %s\n",