[bug report] ASoC: SOF: ipc4-loader: Support for loading external libraries

[Péter Ujfalusi]

Hi Dan,

On 25/10/2022 15:00, Dan Carpenter wrote:
> Hello Peter Ujfalusi,
> 
> The patch 73c091a2fe96: "ASoC: SOF: ipc4-loader: Support for loading
> external libraries" from Oct 20, 2022, leads to the following Smatch
> static checker warning:
> 
> 	sound/soc/sof/ipc4-loader.c:207 sof_ipc4_load_library_by_uuid()
> 	warn: 'payload_offset' unsigned <= 0
> 
> sound/soc/sof/ipc4-loader.c
>     167 static int sof_ipc4_load_library_by_uuid(struct snd_sof_dev *sdev,
>     168                                          unsigned long lib_id, const guid_t *uuid)
>     169 {
>     170         struct sof_ipc4_fw_data *ipc4_data = sdev->private;
>     171         struct sof_ipc4_fw_library *fw_lib;
>     172         const char *fw_filename;
>     173         size_t payload_offset;
>                 ^^^^^^^^^^^^^^^^^^^^^
> 
>     174         int ret, i, err;
>     175 
>     176         if (!sdev->pdata->fw_lib_prefix) {
>     177                 dev_err(sdev->dev,
>     178                         "Library loading is not supported due to not set library path\n");
>     179                 return -EINVAL;
>     180         }
>     181 
>     182         if (!ipc4_data->load_library) {
>     183                 dev_err(sdev->dev, "Library loading is not supported on this platform\n");
>     184                 return -EOPNOTSUPP;
>     185         }
>     186 
>     187         fw_lib = devm_kzalloc(sdev->dev, sizeof(*fw_lib), GFP_KERNEL);
>     188         if (!fw_lib)
>     189                 return -ENOMEM;
>     190 
>     191         fw_filename = kasprintf(GFP_KERNEL, "%s/%pUL.bin",
>     192                                 sdev->pdata->fw_lib_prefix, uuid);
>     193         if (!fw_filename) {
>     194                 ret = -ENOMEM;
>     195                 goto free_fw_lib;
>     196         }
>     197 
>     198         ret = request_firmware(&fw_lib->sof_fw.fw, fw_filename, sdev->dev);
>     199         if (ret < 0) {
>     200                 dev_err(sdev->dev, "Library file '%s' is missing\n", fw_filename);
>     201                 goto free_filename;
>     202         } else {
>     203                 dev_dbg(sdev->dev, "Library file '%s' loaded\n", fw_filename);
>     204         }
>     205 
>     206         payload_offset = sof_ipc4_fw_parse_ext_man(sdev, fw_lib);
> --> 207         if (payload_offset <= 0) {
>                     ^^^^^^^^^^^^^^^^^^^
> sof_ipc4_fw_parse_ext_man() returns negative error codes but as size_t.
> It should just return int.  If it returns > INT_MAX that can't work on
> 32bit systems.

Right, this is not looking good.

I think the root of the issue is:
61bafd1c4571 ("ASoC: SOF: Introduce IPC dependent ops for firmware
handling, loading")

Where I have size_t as return value for the parse_ext_manifest callback...

Let me send a quick fix for this alone and prepare a bigger one to sort
out the rest.

>     208                 if (!payload_offset)
>     209                         ret = -EINVAL;
>     210                 else
>     211                         ret = payload_offset;
>     212 
>     213                 goto release;
>     214         }
>     215 
>     216         fw_lib->sof_fw.payload_offset = payload_offset;
>     217         fw_lib->id = lib_id;
>     218 
>     219         /* Fix up the module ID numbers within the library */
>     220         for (i = 0; i < fw_lib->num_modules; i++)
>     221                 fw_lib->modules[i].man4_module_entry.id |= (lib_id << SOF_IPC4_MOD_LIB_ID_SHIFT);
>     222 
>     223         /*
>     224          * Make sure that the DSP is booted and stays up while attempting the
>     225          * loading the library for the first time
>     226          */
>     227         ret = pm_runtime_resume_and_get(sdev->dev);
>     228         if (ret < 0 && ret != -EACCES) {
>     229                 dev_err_ratelimited(sdev->dev, "%s: pm_runtime resume failed: %d\n",
>     230                                     __func__, ret);
>     231                 goto release;
>     232         }
>     233 
>     234         ret = ipc4_data->load_library(sdev, fw_lib, false);
>     235 
>     236         pm_runtime_mark_last_busy(sdev->dev);
>     237         err = pm_runtime_put_autosuspend(sdev->dev);
>     238         if (err < 0)
>     239                 dev_err_ratelimited(sdev->dev, "%s: pm_runtime idle failed: %d\n",
>     240                                     __func__, err);
>     241 
>     242         if (ret)
>     243                 goto release;
>     244 
>     245         ret = xa_insert(&ipc4_data->fw_lib_xa, lib_id, fw_lib, GFP_KERNEL);
>     246         if (unlikely(ret))
>     247                 goto release;
>     248 
>     249         kfree(fw_filename);
>     250 
>     251         return 0;
>     252 
>     253 release:
>     254         release_firmware(fw_lib->sof_fw.fw);
>     255         /* Allocated within sof_ipc4_fw_parse_ext_man() */
>     256         devm_kfree(sdev->dev, fw_lib->modules);
>     257 free_filename:
>     258         kfree(fw_filename);
>     259 free_fw_lib:
>     260         devm_kfree(sdev->dev, fw_lib);
>     261 
>     262         return ret;
>     263 }
> 
> regards,
> dan carpenter

-- 
Péter