Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <74ba8770-6174-4820-8dc4-e97e5814e665@linux.intel.com>
Date:   Tue, 5 Dec 2023 11:44:16 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] ASoC: SOF: Fix null pointer dereference in sof_pci_probe
To:     Kunwu Chan <chentao@kylinos.cn>,
        pierre-louis.bossart@linux.intel.com, lgirdwood@gmail.com,
        yung-chuan.liao@linux.intel.com, ranjani.sridharan@linux.intel.com,
        daniel.baluta@nxp.com, kai.vehmanen@linux.intel.com,
        broonie@kernel.org, perex@perex.cz, tiwai@suse.com,
        chao.song@intel.com
Cc:     kunwu.chan@hotmail.com, sound-open-firmware@alsa-project.org,
        linux-sound@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20231123155424.1275919-1-chentao@kylinos.cn>
 <1701766721668457.408.seg@mailgw>
 <55c00bb3-b795-4698-ba3f-56d10df1882a@kylinos.cn>
Content-Language: en-US
From:   =?UTF-8?Q?P=C3=A9ter_Ujfalusi?= <peter.ujfalusi@linux.intel.com>
In-Reply-To: <55c00bb3-b795-4698-ba3f-56d10df1882a@kylinos.cn>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


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