Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <18a04b79-bbba-2f58-8e9c-5446aa0eb34d@linux.intel.com>
Date:   Mon, 27 Mar 2023 09:20:39 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.8.0
Subject: Re: [PATCH v8] ASoC: tas2781: Add tas2781 driver
Content-Language: en-US
To:     Shenghao Ding <13916275206@139.com>, broonie@kernel.org,
        lgirdwood@gmail.com, perex@perex.cz
Cc:     kevin-lu@ti.com, shenghao-ding@ti.com, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org, x1077012@ti.com, peeyush@ti.com,
        navada@ti.com
References: <20230327105157.21752-1-13916275206@139.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20230327105157.21752-1-13916275206@139.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

The code looks much better, and now that you've replaced all the -1
error codes with -EINVAL or -ENOMEM we can see clearly cases with memory
leaks in the error handling path.

> +	/* Skip the unused prog_size */
> +	offset += (4 * (TASDEVICE_MAXPROGRAM_NUM_KERNEL -
> +		tas_fmw->nr_programs));
> +
> +	if (offset + 4 > fmw->size) {
> +		dev_err(tas_dev->dev, "%s: Configurations error\n", __func__);
> +		offset = -EINVAL;
> +		goto out;
> +	}
> +	tas_fmw->nr_configurations = SMS_HTONL(buf[offset], buf[offset + 1],
> +		buf[offset + 2], buf[offset + 3]);
> +	offset += 4;
> +
> +	/* The max of conf for more than equal to 4 pcs of tas2781s is
> +	 * different from the one less than 4 pcs of tas2781s.
> +	 */

Consider rewording, this comment is just confusing. Not sure what 'pcs'
means here, and "more than equal' and 'one less than' should be replaced
by 'greater than' and 'lower than'.

> +static int fw_parse_data(struct tasdevice_fw *tas_fmw,
> +	struct tasdevice_data *img_data, const struct firmware *fmw,
> +	int offset)
> +{
> +	const unsigned char *data = (unsigned char *)fmw->data;
> +	struct tasdev_blk *blk;
> +	unsigned int i;
> +	int n;
> +
> +	if (offset + 64 > fmw->size) {
> +		dev_err(tas_fmw->dev, "%s: Name error\n", __func__);
> +		offset = -EINVAL;
> +		goto out;
> +	}
> +	memcpy(img_data->name, &data[offset], 64);
> +	offset += 64;
> +
> +	n = strlen((char *)&data[offset]);
> +	n++;
> +	if (offset + n > fmw->size) {
> +		dev_err(tas_fmw->dev, "%s: Description error\n", __func__);
> +		offset = -EINVAL;
> +		goto out;
> +	}
> +	offset += n;
> +
> +	if (offset + 2 > fmw->size) {
> +		dev_err(tas_fmw->dev, "%s: Blocks error\n", __func__);
> +		offset = -EINVAL;
> +		goto out;
> +	}
> +	img_data->nr_blk = SMS_HTONS(data[offset], data[offset + 1]);
> +	offset += 2;
> +
> +	img_data->dev_blks = kcalloc(img_data->nr_blk,
> +		sizeof(struct tasdev_blk), GFP_KERNEL);
> +	if (!img_data->dev_blks) {
> +		offset = -ENOMEM;
> +		goto out;
> +	}
> +	for (i = 0; i < img_data->nr_blk; i++) {
> +		blk = &(img_data->dev_blks[i]);
> +		offset = fw_parse_block_data(tas_fmw, blk, fmw, offset);
> +		if (offset < 0) {
> +			offset = -EINVAL;
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	return offset;

memory leak on img_data->dev_blks, you need to free memory in the error
handling path.

> +}
> +
> +static int fw_parse_calibration_data(struct tasdevice_priv *tas_dev,
> +	struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset)
> +{
> +	struct tasdevice_calibration *calibration;
> +	unsigned char *data = (unsigned char *)fmw->data;
> +	unsigned int i, n;
> +
> +	if (offset + 2 > fmw->size) {
> +		dev_err(tas_dev->dev, "%s: Calibrations error\n", __func__);
> +		offset = -EINVAL;
> +		goto out;
> +	}
> +	tas_fmw->nr_calibrations = SMS_HTONS(data[offset], data[offset + 1]);
> +	offset += 2;
> +
> +	if (tas_fmw->nr_calibrations != 1) {
> +		dev_err(tas_dev->dev,
> +			"%s: only support one calibraiton(%d)!\n",
> +			__func__, tas_fmw->nr_calibrations);
> +		goto out;
> +	}
> +
> +	tas_fmw->calibrations = kcalloc(tas_fmw->nr_calibrations,
> +		sizeof(struct tasdevice_calibration), GFP_KERNEL);
> +	if (!tas_fmw->calibrations) {
> +		offset = -ENOMEM;
> +		goto out;
> +	}
> +	for (i = 0; i < tas_fmw->nr_calibrations; i++) {
> +		if (offset + 64 > fmw->size) {
> +			dev_err(tas_dev->dev, "Calibrations error\n");
> +			offset = -EINVAL;

memory leak on tas_fmw->calibrations, you need to change the error
handling path.

> +			goto out;
> +		}
> +		calibration = &(tas_fmw->calibrations[i]);
> +		offset += 64;
> +
> +		n = strlen((char *)&data[offset]);
> +		n++;
> +		if (offset + n > fmw->size) {
> +			dev_err(tas_dev->dev, "Description err\n");
> +			offset = -EINVAL;
> +			goto out;
> +		}
> +		offset += n;
> +
> +		if (offset + 1 > fmw->size) {
> +			dev_err(tas_dev->dev, "%s: Prog err, offset = %d\n",
> +				__func__, offset);
> +			offset = -EINVAL;
> +			goto out;
> +		}
> +		offset++;
> +
> +		if (offset + 1 > fmw->size) {
> +			dev_err(tas_dev->dev, "%s: Conf err, offset = %d\n",
> +				__func__, offset);
> +			offset = -EINVAL;
> +			goto out;
> +		}
> +		offset++;
> +
> +		offset = fw_parse_data(tas_fmw, &(calibration->dev_data), fmw,
> +			offset);
> +		if (offset < 0)
> +			goto out;
> +	}
> +
> +out:
> +	return offset;
> +}
> +
> +static int fw_parse_program_data(struct tasdevice_priv *tas_dev,
> +	struct tasdevice_fw *tas_fmw, const struct firmware *fmw, int offset)
> +{
> +	unsigned char *buf = (unsigned char *)fmw->data;
> +	struct tasdevice_prog *program;
> +	int i;
> +
> +	if (offset + 2 > fmw->size) {
> +		dev_err(tas_dev->dev, "%s: File Size error\n", __func__);
> +		offset = -EINVAL;
> +		goto out;
> +	}
> +	tas_fmw->nr_programs = SMS_HTONS(buf[offset], buf[offset + 1]);
> +	offset += 2;
> +
> +	if (tas_fmw->nr_programs == 0) {
> +		/*Not error in calibration Data file, return directly*/
> +		dev_info(tas_dev->dev, "%s: No Programs data, maybe calbin\n",
> +			__func__);
> +		goto out;
> +	}
> +
> +	tas_fmw->programs =
> +		kcalloc(tas_fmw->nr_programs, sizeof(struct tasdevice_prog),
> +			GFP_KERNEL);
> +	if (!tas_fmw->programs) {
> +		offset = -ENOMEM;
> +		goto out;
> +	}
> +	for (i = 0; i < tas_fmw->nr_programs; i++) {
> +		int n = 0;
> +
> +		program = &(tas_fmw->programs[i]);
> +		if (offset + 64 > fmw->size) {
> +			dev_err(tas_dev->dev, "%s: mpName error\n", __func__);
> +			offset = -EINVAL;
> +			goto out;

and memory leak here as well.

Stopping the review here, please revisit the error handling. you
probably need two labels when memory is allocated, and a kfree() for one
of the two.