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