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

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

[Kunwu Chan]

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@kylinos.cn>
---
 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",
-- 
2.34.1

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

[Péter Ujfalusi]

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@kylinos.cn>

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

On Thu, Nov 23, 2023 at 11:54:24PM +0800, Kunwu Chan wrote:
> devm_kasprintf() returns a pointer to dynamically allocated memory
> which can be NULL upon failure.

In addition to the issues others mentioned this doesn't apply against
current code, please check and resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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@kylinos.cn>
> 
> 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",
> 

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

[Kunwu Chan]

Thanks for your reply.
I'll check the patch and modify it by a better way.

Thanks again,
Kunwu

On 2023/12/4 20:38, Mark Brown wrote:
> On Thu, Nov 23, 2023 at 11:54:24PM +0800, Kunwu Chan wrote:
>> devm_kasprintf() returns a pointer to dynamically allocated memory
>> which can be NULL upon failure.
> 
> In addition to the issues others mentioned this doesn't apply against
> current code, please check and resend.

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@kylinos.cn>
>>
>> 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