| 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: <34017b95-5bb3-79a0-e819-17ee113fa6c8@linux.intel.com>
Date: Tue, 28 Mar 2023 12:11:25 +0200
From: Amadeusz Sławiński
<amadeuszx.slawinski@...ux.intel.com>
To: Shenghao Ding <13916275206@....com>, broonie@...nel.org,
lgirdwood@...il.com, perex@...ex.cz,
pierre-louis.bossart@...ux.intel.com
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 v9] ASoC: tas2781: Add tas2781 driver
On 3/28/2023 11:49 AM, Shenghao Ding wrote:
> Create tas2781 driver.
>
> Signed-off-by: Shenghao Ding <13916275206@....com>
>
> ---
> Changes in v9:
> - rewording some commets
> - Add comments on fimware parsing error handling -- all the memory resources
> will be released in the end of tasdevice_rca_ready (fw_parse_data,
> fw_parse_program_data & fw_parse_configuration_data).
> - Add comments on fw_parse_calibration_data -- DSP can still work with default
> calibrated data, memory resource related to calibrated data will be released
> in the tasdevice_codec_remove.
> modified: sound/soc/codecs/Kconfig
> modified: sound/soc/codecs/Makefile
> new file: sound/soc/codecs/tas2781-dsp.c
> new file: sound/soc/codecs/tas2781-dsp.h
> new file: sound/soc/codecs/tas2781-i2c.c
> new file: sound/soc/codecs/tas2781.h
> ---
...
> +
> +static int fw_parse_block_data_kernel(struct tasdevice_fw *tas_fmw,
> + struct tasdev_blk *block, const struct firmware *fmw, int offset)
> +{
> + const unsigned char *data = fmw->data;
> +
> + if (offset + 4 > fmw->size) {
> + dev_err(tas_fmw->dev, "%s: File Size error\n", __func__);
> + offset = -EINVAL;
> + goto out;
> + }
> + block->type = SMS_HTONL(data[offset], data[offset + 1],
> + data[offset + 2], data[offset + 3]);
> + offset += 4;
> +
> + if (offset + 1 > fmw->size) {
> + dev_err(tas_fmw->dev, "%s: PChkSumPresent error\n", __func__);
> + offset = -EINVAL;
> + goto out;
> + }
> + block->is_pchksum_present = data[offset];
> + offset++;
> +
> + if (offset + 1 > fmw->size) {
> + dev_err(tas_fmw->dev, "%s: mnPChkSum error\n", __func__);
> + offset = -EINVAL;
> + goto out;
> + }
> + block->pchksum = data[offset];
> + offset++;
> +
> + if (offset + 1 > fmw->size) {
> + dev_err(tas_fmw->dev, "%s: YChkSumPresent error\n", __func__);
> + offset = -EINVAL;
> + goto out;
> + }
> + block->is_ychksum_present = data[offset];
> + offset++;
> +
> + if (offset + 1 > fmw->size) {
> + dev_err(tas_fmw->dev, "%s: mnYChkSum error\n", __func__);
> + offset = -EINVAL;
> + goto out;
> + }
> + block->ychksum = data[offset];
> + offset++;
> +
> + if (offset + 4 > fmw->size) {
> + dev_err(tas_fmw->dev, "%s: blk_size error\n", __func__);
> + offset = -EINVAL;
> + goto out;
> + }
> + block->blk_size = SMS_HTONL(data[offset], data[offset + 1],
> + data[offset + 2], data[offset + 3]);
> + offset += 4;
> +
> + if (offset + 4 > fmw->size) {
> + dev_err(tas_fmw->dev, "%s: nSublocks error\n", __func__);
> + offset = -EINVAL;
> + goto out;
> + }
> + block->nr_subblocks = SMS_HTONL(data[offset], data[offset + 1],
> + data[offset + 2], data[offset + 3]);
> + offset += 4;
> +
> + if (offset + block->blk_size > fmw->size) {
> + dev_err(tas_fmw->dev, "%s: nSublocks error\n", __func__);
> + offset = -EINVAL;
> + goto out;
> + }
> + block->data = kmemdup(&data[offset], block->blk_size, GFP_KERNEL);
> + if (!block->data) {
> + offset = -ENOMEM;
> + goto out;
> + }
> + offset += block->blk_size;
> +
> +out:
> + return offset;
> +}
> +
I have a question regarding those
if (offset + x > fmw->size) {
do error
}
offset += x;
Can you instead of doing check before every assignment, just check once
if you have enough data to parse, before parsing whole piece of data
instead?
For above function it would be something like:
static int fw_parse_block_data_kernel(struct tasdevice_fw *tas_fmw,
struct tasdev_blk *block, const struct firmware *fmw, int offset)
{
const unsigned char *data = fmw->data;
if (offset + 16 + block->blk_size > fmw->size) {
dev_err(tas_fmw->dev, "%s: File Size error\n", __func__);
offset = -EINVAL;
goto out;
}
block->type = SMS_HTONL(data[offset], data[offset + 1],
data[offset + 2], data[offset + 3]);
block->is_pchksum_present = data[offset + 4];
block->pchksum = data[offset + 5];
block->is_ychksum_present = data[offset + 6];
block->ychksum = data[offset + 7];
block->blk_size = SMS_HTONL(data[offset + 8], data[offset + 9],
data[offset + 10], data[offset + 11]);
block->nr_subblocks = SMS_HTONL(data[offset + 12], data[offset + 13],
data[offset + 14], data[offset + 15]);
offset += 16;
block->data = kmemdup(&data[offset], block->blk_size, GFP_KERNEL);
if (!block->data) {
offset = -ENOMEM;
goto out;
}
offset += block->blk_size;
out:
return offset;
}
Additionally if you defined 'struct tasdev_blk' to reflect data which
you copy here it can probably be simplified further to simple memcpy,
instead of doing field by field assignments, which would reduce amount
of code significantly and make it easier to read.
Above applies to all similar parsing in the patch.
> +static int fw_parse_data_kernel(struct tasdevice_fw *tas_fmw,
> + struct tasdevice_data *img_data, const struct firmware *fmw,
> + int offset)
> +{
> + const unsigned char *data = fmw->data;
> + struct tasdev_blk *blk;
> + unsigned int i;
> +
...
> +
> +#define SMS_HTONS(a, b) ((((a)&0x00FF)<<8) | ((b)&0x00FF))
> +#define SMS_HTONL(a, b, c, d) ((((a)&0x000000FF)<<24) | \
> + (((b)&0x000000FF)<<16) | (((c)&0x000000FF)<<8) | \
> + ((d)&0x000000FF))
> +
Kernel has htons and htonl (in
source/include/linux/byteorder/generic.h), but I assume that this means
that your FW may use different endianness than host cpu (big endian?),
so I would suggest using be16_to_cpu and be32_to_cpu macros instead, as
this should be more understandable in this case.
Powered by blists - more mailing lists