Re: [PATCH v5 09/14] ASoC: SOF: Add firmware loader support

[Takashi Iwai <tiwai@suse.de>]

On Thu, 21 Mar 2019 17:10:11 +0100,
Pierre-Louis Bossart wrote:
> 
> +static int get_ext_windows(struct snd_sof_dev *sdev,
> +			   struct sof_ipc_ext_data_hdr *ext_hdr)
> +{
> +	struct sof_ipc_window *w = (struct sof_ipc_window *)ext_hdr;

In general it's safer to use container_of() than the forced cast, and...

> +/* parse the extended FW boot data structures from FW boot message */
> +int snd_sof_fw_parse_ext_data(struct snd_sof_dev *sdev, u32 bar, u32 offset)
> +{
> +	struct sof_ipc_ext_data_hdr *ext_hdr;
> +	void *ext_data;
> +	int ret = 0;
> +
> +	ext_data = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!ext_data)
> +		return -ENOMEM;
> +
> +	/* get first header */
> +	snd_sof_dsp_block_read(sdev, bar, offset, ext_data,
> +			       sizeof(*ext_hdr));
> +	ext_hdr = (struct sof_ipc_ext_data_hdr *)ext_data;

The cast isn't needed from a void pointer.  The whole SOF codes have
way too many unneeded cast.  Let's reduce.

> +/* generic module parser for mmaped DSPs */
> +int snd_sof_parse_module_memcpy(struct snd_sof_dev *sdev,
> +				struct snd_sof_mod_hdr *module)
> +{
> +	struct snd_sof_blk_hdr *block;
> +	int count;
> +	u32 offset;
> +	size_t remaining;
> +
> +	dev_dbg(sdev->dev, "new module size 0x%x blocks 0x%x type 0x%x\n",
> +		module->size, module->num_blocks, module->type);
> +
> +	block = (struct snd_sof_blk_hdr *)((u8 *)module + sizeof(*module));
> +
> +	/* module->size doesn't include header size */
> +	remaining = module->size;
> +	for (count = 0; count < module->num_blocks; count++) {
> +		/* minus header size of block */
> +		remaining -= sizeof(*block);
> +		if (remaining < block->size) {
> +			dev_err(sdev->dev, "error: not enough data remaining\n");
> +			return -EINVAL;
> +		}

remaining is unsigned, so a negative check doesn't work here.
Hence you need the explicit underflow check.

> +static int load_modules(struct snd_sof_dev *sdev, const struct firmware *fw)
> +{
> +	struct snd_sof_fw_header *header;
> +	struct snd_sof_mod_hdr *module;
> +	int (*load_module)(struct snd_sof_dev *sof_dev,
> +			   struct snd_sof_mod_hdr *hdr);
> +	int ret, count;
> +	size_t remaining;
> +
> +	header = (struct snd_sof_fw_header *)fw->data;
> +	load_module = sof_ops(sdev)->load_module;
> +	if (!load_module)
> +		return -EINVAL;
> +
> +	/* parse each module */
> +	module = (struct snd_sof_mod_hdr *)((u8 *)(fw->data) + sizeof(*header));
> +	remaining = fw->size - sizeof(*header);

The size check should be more strict (e.g. here remaining might become
negative).

> +int snd_sof_load_firmware_raw(struct snd_sof_dev *sdev)
> +{
> +	struct snd_sof_pdata *plat_data = sdev->pdata;
> +	const char *fw_filename;
> +	int ret;
> +
> +	/* set code loading condition to true */
> +	sdev->code_loading = 1;
> +
> +	/* Don't request firmware again if firmware is already requested */
> +	if (plat_data->fw)
> +		return 0;
> +
> +	fw_filename = devm_kasprintf(sdev->dev, GFP_KERNEL,
> +				     "%s/%s",
> +				     plat_data->fw_filename_prefix,
> +				     plat_data->fw_filename);
> +	if (!fw_filename)
> +		return -ENOMEM;
> +
> +	ret = request_firmware(&plat_data->fw, fw_filename, sdev->dev);
> +
> +	if (ret < 0) {
> +		dev_err(sdev->dev, "error: request firmware %s failed err: %d\n",
> +			fw_filename, ret);
> +	}
> +	return ret;

Hrm, any need of devm_kasprintf() here?  That is, do we need to keep
this string after the call of request_firmware()?  It doesn't make
sense if we need it only temporarily.


thanks,

Takashi