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

[Péter Ujfalusi]

On 05/12/2023 11:10, Kunwu Chan wrote:
> 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?

The code has been rewritten and all of these issues have been
eliminated, fixed in core SOF.

> 
> 
> 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",
>>

-- 
Péter