[PATCH 0/5] ASoC: SOF: fix kcontrol size checks
Series that fixes checks for 'size' in kcontrol get/put ext_bytes methods for SOF. The gaps in these checks were discovered via cppcheck warnings on unused variable values.
Pierre-Louis Bossart (5): ASoC: SOF: control: fix size checks for ext_bytes control .get() ASoC: SOF: control: fix size checks for volatile ext_bytes control .get() ASoC: SOF: control: add size checks for ext_bytes control .put() ASoC: SOF: control: remove const in sizeof() ASoC: SOF: topology: remove const in sizeof()
sound/soc/sof/control.c | 53 +++++++++++++++++++++++++++++++--------- sound/soc/sof/topology.c | 2 +- 2 files changed, 43 insertions(+), 12 deletions(-)
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
cppcheck complains twice:
sound/soc/sof/control.c:436:2: style: Assignment of function parameter has no effect outside the function. [uselessAssignmentArg] size -= sizeof(const struct snd_ctl_tlv); ^
sound/soc/sof/control.c:436:7: style: Variable 'size' is assigned a value that is never used. [unreadVariable] size -= sizeof(const struct snd_ctl_tlv);
Somehow we dropped the checks for the size argument when upstreaming the code, somewhere between v5 and v6.
Re-add a size check to avoid providing userspace with more data that it asked for.
Also fix all error codes, we should return -ENOSPC instead of -EINVAL.
Fixes: c3078f5397046 ('ASoC: SOF: Add Sound Open Firmware KControl support') Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/control.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sof/control.c b/sound/soc/sof/control.c index 58f8c998e6af..8d499d0e331d 100644 --- a/sound/soc/sof/control.c +++ b/sound/soc/sof/control.c @@ -432,7 +432,9 @@ int snd_sof_bytes_ext_get(struct snd_kcontrol *kcontrol, * Decrement the limit by ext bytes header size to * ensure the user space buffer is not exceeded. */ - size -= sizeof(const struct snd_ctl_tlv); + if (size < sizeof(struct snd_ctl_tlv)) + return -ENOSPC; + size -= sizeof(struct snd_ctl_tlv);
/* set the ABI header values */ cdata->data->magic = SOF_ABI_MAGIC; @@ -448,6 +450,10 @@ int snd_sof_bytes_ext_get(struct snd_kcontrol *kcontrol,
data_size = cdata->data->size + sizeof(const struct sof_abi_hdr);
+ /* make sure we don't exceed size provided by user space for data */ + if (data_size > size) + return -ENOSPC; + header.numid = scontrol->cmd; header.length = data_size; if (copy_to_user(tlvd, &header, sizeof(const struct snd_ctl_tlv)))
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Mirror addition of checks for regular ext_bytes controls.
Fixes: 783560d02dd61 ('ASoC: SOF: Implement snd_sof_bytes_ext_volatile_get kcontrol IO') Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/control.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/sound/soc/sof/control.c b/sound/soc/sof/control.c index 8d499d0e331d..9465611156d5 100644 --- a/sound/soc/sof/control.c +++ b/sound/soc/sof/control.c @@ -369,6 +369,14 @@ int snd_sof_bytes_ext_volatile_get(struct snd_kcontrol *kcontrol, unsigned int _ int ret; int err;
+ /* + * Decrement the limit by ext bytes header size to + * ensure the user space buffer is not exceeded. + */ + if (size < sizeof(struct snd_ctl_tlv)) + return -ENOSPC; + size -= sizeof(struct snd_ctl_tlv); + ret = pm_runtime_get_sync(scomp->dev); if (ret < 0 && ret != -EACCES) { dev_err_ratelimited(scomp->dev, "error: bytes_ext get failed to resume %d\n", ret); @@ -396,6 +404,12 @@ int snd_sof_bytes_ext_volatile_get(struct snd_kcontrol *kcontrol, unsigned int _
data_size = cdata->data->size + sizeof(const struct sof_abi_hdr);
+ /* make sure we don't exceed size provided by user space for data */ + if (data_size > size) { + ret = -ENOSPC; + goto out; + } + header.numid = scontrol->cmd; header.length = data_size; if (copy_to_user(tlvd, &header, sizeof(const struct snd_ctl_tlv))) {
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Make sure the TLV header and size are consistent before copying from userspace.
Fixes: c3078f5397046 ('ASoC: SOF: Add Sound Open Firmware KControl support') Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/control.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/sound/soc/sof/control.c b/sound/soc/sof/control.c index 9465611156d5..0352d2b61358 100644 --- a/sound/soc/sof/control.c +++ b/sound/soc/sof/control.c @@ -300,6 +300,10 @@ int snd_sof_bytes_ext_put(struct snd_kcontrol *kcontrol, const struct snd_ctl_tlv __user *tlvd = (const struct snd_ctl_tlv __user *)binary_data;
+ /* make sure we have at least a header */ + if (size < sizeof(struct snd_ctl_tlv)) + return -EINVAL; + /* * The beginning of bytes data contains a header from where * the length (as bytes) is needed to know the correct copy @@ -308,6 +312,13 @@ int snd_sof_bytes_ext_put(struct snd_kcontrol *kcontrol, if (copy_from_user(&header, tlvd, sizeof(const struct snd_ctl_tlv))) return -EFAULT;
+ /* make sure TLV info is consistent */ + if (header.length + sizeof(struct snd_ctl_tlv) > size) { + dev_err_ratelimited(scomp->dev, "error: inconsistent TLV, data %d + header %zu > %d\n", + header.length, sizeof(struct snd_ctl_tlv), size); + return -EINVAL; + } + /* be->max is coming from topology */ if (header.length > be->max) { dev_err_ratelimited(scomp->dev, "error: Bytes data size %d exceeds max %d.\n",
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
We should only use the type, the const attribute makes no sense in sizeof().
Reported-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/control.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/sound/soc/sof/control.c b/sound/soc/sof/control.c index 0352d2b61358..056c86ad5a47 100644 --- a/sound/soc/sof/control.c +++ b/sound/soc/sof/control.c @@ -309,7 +309,7 @@ int snd_sof_bytes_ext_put(struct snd_kcontrol *kcontrol, * the length (as bytes) is needed to know the correct copy * length of data from tlvd->tlv. */ - if (copy_from_user(&header, tlvd, sizeof(const struct snd_ctl_tlv))) + if (copy_from_user(&header, tlvd, sizeof(struct snd_ctl_tlv))) return -EFAULT;
/* make sure TLV info is consistent */ @@ -351,7 +351,7 @@ int snd_sof_bytes_ext_put(struct snd_kcontrol *kcontrol, }
/* be->max has been verified to be >= sizeof(struct sof_abi_hdr) */ - if (cdata->data->size > be->max - sizeof(const struct sof_abi_hdr)) { + if (cdata->data->size > be->max - sizeof(struct sof_abi_hdr)) { dev_err_ratelimited(scomp->dev, "error: Mismatch in ABI data size (truncated?).\n"); return -EINVAL; } @@ -405,15 +405,15 @@ int snd_sof_bytes_ext_volatile_get(struct snd_kcontrol *kcontrol, unsigned int _ goto out;
/* check data size doesn't exceed max coming from topology */ - if (cdata->data->size > be->max - sizeof(const struct sof_abi_hdr)) { + if (cdata->data->size > be->max - sizeof(struct sof_abi_hdr)) { dev_err_ratelimited(scomp->dev, "error: user data size %d exceeds max size %zu.\n", cdata->data->size, - be->max - sizeof(const struct sof_abi_hdr)); + be->max - sizeof(struct sof_abi_hdr)); ret = -EINVAL; goto out; }
- data_size = cdata->data->size + sizeof(const struct sof_abi_hdr); + data_size = cdata->data->size + sizeof(struct sof_abi_hdr);
/* make sure we don't exceed size provided by user space for data */ if (data_size > size) { @@ -423,7 +423,7 @@ int snd_sof_bytes_ext_volatile_get(struct snd_kcontrol *kcontrol, unsigned int _
header.numid = scontrol->cmd; header.length = data_size; - if (copy_to_user(tlvd, &header, sizeof(const struct snd_ctl_tlv))) { + if (copy_to_user(tlvd, &header, sizeof(struct snd_ctl_tlv))) { ret = -EFAULT; goto out; } @@ -466,14 +466,14 @@ int snd_sof_bytes_ext_get(struct snd_kcontrol *kcontrol, cdata->data->abi = SOF_ABI_VERSION;
/* check data size doesn't exceed max coming from topology */ - if (cdata->data->size > be->max - sizeof(const struct sof_abi_hdr)) { + if (cdata->data->size > be->max - sizeof(struct sof_abi_hdr)) { dev_err_ratelimited(scomp->dev, "error: user data size %d exceeds max size %zu.\n", cdata->data->size, - be->max - sizeof(const struct sof_abi_hdr)); + be->max - sizeof(struct sof_abi_hdr)); return -EINVAL; }
- data_size = cdata->data->size + sizeof(const struct sof_abi_hdr); + data_size = cdata->data->size + sizeof(struct sof_abi_hdr);
/* make sure we don't exceed size provided by user space for data */ if (data_size > size) @@ -481,7 +481,7 @@ int snd_sof_bytes_ext_get(struct snd_kcontrol *kcontrol,
header.numid = scontrol->cmd; header.length = data_size; - if (copy_to_user(tlvd, &header, sizeof(const struct snd_ctl_tlv))) + if (copy_to_user(tlvd, &header, sizeof(struct snd_ctl_tlv))) return -EFAULT;
if (copy_to_user(tlvd->tlv, cdata->data, data_size))
From: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
We should only use the type, the const attribute makes no sense in sizeof().
Reported-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Guennadi Liakhovetski guennadi.liakhovetski@linux.intel.com Signed-off-by: Kai Vehmanen kai.vehmanen@linux.intel.com --- sound/soc/sof/topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c index eaa1122d5a68..1e42eb32f3b7 100644 --- a/sound/soc/sof/topology.c +++ b/sound/soc/sof/topology.c @@ -1201,7 +1201,7 @@ static int sof_control_load_bytes(struct snd_soc_component *scomp, ret = -EINVAL; goto out_free; } - if (cdata->data->size + sizeof(const struct sof_abi_hdr) != + if (cdata->data->size + sizeof(struct sof_abi_hdr) != le32_to_cpu(control->priv.size)) { dev_err(scomp->dev, "error: Conflict in bytes vs. priv size.\n");
On Mon, 21 Sep 2020 14:08:09 +0300, Kai Vehmanen wrote:
Series that fixes checks for 'size' in kcontrol get/put ext_bytes methods for SOF. The gaps in these checks were discovered via cppcheck warnings on unused variable values.
Pierre-Louis Bossart (5): ASoC: SOF: control: fix size checks for ext_bytes control .get() ASoC: SOF: control: fix size checks for volatile ext_bytes control .get() ASoC: SOF: control: add size checks for ext_bytes control .put() ASoC: SOF: control: remove const in sizeof() ASoC: SOF: topology: remove const in sizeof()
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/5] ASoC: SOF: control: fix size checks for ext_bytes control .get() commit: 3331bcd6a2f2dbe9c1fa764df695422c99e2f1fb [2/5] ASoC: SOF: control: fix size checks for volatile ext_bytes control .get() commit: ec5a97624a8de4f44b090cf53bd48c05458e0b17 [3/5] ASoC: SOF: control: add size checks for ext_bytes control .put() commit: 2ca210112ad91880d2d5a3f85fecc838600afbce [4/5] ASoC: SOF: control: remove const in sizeof() (no commit info) [5/5] ASoC: SOF: topology: remove const in sizeof() (no commit info)
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
Hi Mark,
On Tue, 22 Sep 2020, Mark Brown wrote:
[3/5] ASoC: SOF: control: add size checks for ext_bytes control .put() commit: 2ca210112ad91880d2d5a3f85fecc838600afbce [4/5] ASoC: SOF: control: remove const in sizeof() (no commit info) [5/5] ASoC: SOF: topology: remove const in sizeof() (no commit info)
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
I wonder what happened here...? Patches 4 and 5 didn't end up applied although they were in the sent series. I can send them again no prob, but wondering if there was something wrong in the original series, so I can avoid the problem in the future.
Br, Kai
On Thu, Sep 24, 2020 at 08:45:30AM +0300, Kai Vehmanen wrote:
I wonder what happened here...? Patches 4 and 5 didn't end up applied although they were in the sent series. I can send them again no prob, but wondering if there was something wrong in the original series, so I can avoid the problem in the future.
Do those patches actually apply to for-5.10 or are they correcting issues that only exist in for-5.9?
Hey,
On Thu, 24 Sep 2020, Mark Brown wrote:
On Thu, Sep 24, 2020 at 08:45:30AM +0300, Kai Vehmanen wrote:
I wonder what happened here...? Patches 4 and 5 didn't end up applied although they were in the sent series. I can send them again no prob, but wondering if there was something wrong in the original series, so I can avoid the problem in the future.
Do those patches actually apply to for-5.10 or are they correcting issues that only exist in for-5.9?
yes, the series was based on broonie/for-5.10 for sending, and I tested again and both of the dropped patches still apply on top of for-5.10. They do not apply cleanly on top of for-5.9.
Br, Kai
On Thu, Sep 24, 2020 at 02:30:23PM +0300, Kai Vehmanen wrote:
On Thu, 24 Sep 2020, Mark Brown wrote:
Do those patches actually apply to for-5.10 or are they correcting issues that only exist in for-5.9?
yes, the series was based on broonie/for-5.10 for sending, and I tested again and both of the dropped patches still apply on top of for-5.10. They do not apply cleanly on top of for-5.9.
Well, that's the only thing I can think of - that git thought they didn't actaully have any changes in them when it tried to apply them.
Hi,
On Thu, 24 Sep 2020, Mark Brown wrote:
On Thu, Sep 24, 2020 at 02:30:23PM +0300, Kai Vehmanen wrote:
yes, the series was based on broonie/for-5.10 for sending, and I tested again and both of the dropped patches still apply on top of for-5.10. They
Well, that's the only thing I can think of - that git thought they didn't actaully have any changes in them when it tried to apply them.
ack. I'll resend them -- let's see if they go through the system this time. :) In any case these were just coding style updates, so not urgent in any case.
Br, Kai
participants (2)
-
Kai Vehmanen -
Mark Brown