| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <18a04b79-bbba-2f58-8e9c-5446aa0eb34d@linux.intel.com>
Date: Mon, 27 Mar 2023 09:20:39 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Shenghao Ding <13916275206@....com>, broonie@...nel.org,
lgirdwood@...il.com, perex@...ex.cz
Cc: kevin-lu@...com, shenghao-ding@...com, alsa-devel@...a-project.org,
linux-kernel@...r.kernel.org, x1077012@...com, peeyush@...com,
navada@...com
Subject: Re: [PATCH v8] ASoC: tas2781: Add tas2781 driver
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.
Powered by blists - more mailing lists