[PATCH 0/5] ASoC: SOF: fix kcontrol size checks

[PATCH 0/5] ASoC: SOF: fix kcontrol size checks

[Kai Vehmanen]

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(-)

-- 
2.27.0

[PATCH 1/5] ASoC: SOF: control: fix size checks for ext_bytes control .get()

[Kai Vehmanen]

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)))
-- 
2.27.0

[PATCH 2/5] ASoC: SOF: control: fix size checks for volatile ext_bytes control .get()

[Kai Vehmanen]

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))) {
-- 
2.27.0

[PATCH 3/5] ASoC: SOF: control: add size checks for ext_bytes control .put()

[Kai Vehmanen]

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",
-- 
2.27.0

[PATCH 4/5] ASoC: SOF: control: remove const in sizeof()

[Kai Vehmanen]

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))
-- 
2.27.0

[PATCH 5/5] ASoC: SOF: topology: remove const in sizeof()

[Kai Vehmanen]

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");
-- 
2.27.0

Re: [PATCH 0/5] ASoC: SOF: fix kcontrol size checks

[Mark Brown]

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

Re: [PATCH 0/5] ASoC: SOF: fix kcontrol size checks

[Kai Vehmanen]

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

Re: [PATCH 0/5] ASoC: SOF: fix kcontrol size checks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 435 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] ASoC: SOF: fix kcontrol size checks

[Kai Vehmanen]

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

Re: [PATCH 0/5] ASoC: SOF: fix kcontrol size checks

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] ASoC: SOF: fix kcontrol size checks

[Kai Vehmanen]

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