[PATCH v1 0/3] ASoC: Add bounds checking for written values

[PATCH v1 0/3] ASoC: Add bounds checking for written values

[Mark Brown]

This series adds verification that values written to registers are in
bounds for controls since the core doesn't validate for us.

Mark Brown (3):
  ASoC: ops: Reject out of bounds values in snd_soc_put_volsw()
  ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()
  ASoC: ops: Reject out of bounds values in snd_soc_put_xr_sx()

 sound/soc/soc-ops.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)


base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07
-- 
2.30.2

[PATCH v1 1/3] ASoC: ops: Reject out of bounds values in snd_soc_put_volsw()

[Mark Brown]

We don't currently validate that the values being set are within the range
we advertised to userspace as being valid, do so and reject any values
that are out of range.

Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 sound/soc/soc-ops.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index 08eaa9ddf191..fbe5d326b0f2 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -316,13 +316,27 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 	if (sign_bit)
 		mask = BIT(sign_bit + 1) - 1;
 
-	val = ((ucontrol->value.integer.value[0] + min) & mask);
+	val = ucontrol->value.integer.value[0];
+	if (mc->platform_max && val > mc->platform_max)
+		return -EINVAL;
+	if (val > max - min)
+		return -EINVAL;
+	if (val < 0)
+		return -EINVAL;
+	val = (val + min) & mask;
 	if (invert)
 		val = max - val;
 	val_mask = mask << shift;
 	val = val << shift;
 	if (snd_soc_volsw_is_stereo(mc)) {
-		val2 = ((ucontrol->value.integer.value[1] + min) & mask);
+		val2 = ucontrol->value.integer.value[1];
+		if (mc->platform_max && val2 > mc->platform_max)
+			return -EINVAL;
+		if (val2 > max - min)
+			return -EINVAL;
+		if (val2 < 0)
+			return -EINVAL;
+		val2 = (val2 + min) & mask;
 		if (invert)
 			val2 = max - val2;
 		if (reg == reg2) {
-- 
2.30.2

[PATCH v1 2/3] ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()

[Mark Brown]

We don't currently validate that the values being set are within the range
we advertised to userspace as being valid, do so and reject any values
that are out of range.

Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 sound/soc/soc-ops.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index fbe5d326b0f2..c31e63b27193 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -423,8 +423,15 @@ int snd_soc_put_volsw_sx(struct snd_kcontrol *kcontrol,
 	int err = 0;
 	unsigned int val, val_mask;
 
+	val = ucontrol->value.integer.value[0];
+	if (mc->platform_max && val > mc->platform_max)
+		return -EINVAL;
+	if (val > max - min)
+		return -EINVAL;
+	if (val < 0)
+		return -EINVAL;
 	val_mask = mask << shift;
-	val = (ucontrol->value.integer.value[0] + min) & mask;
+	val = (val + min) & mask;
 	val = val << shift;
 
 	err = snd_soc_component_update_bits(component, reg, val_mask, val);
-- 
2.30.2

[PATCH v1 3/3] ASoC: ops: Reject out of bounds values in snd_soc_put_xr_sx()

[Mark Brown]

We don't currently validate that the values being set are within the range
we advertised to userspace as being valid, do so and reject any values
that are out of range.

Signed-off-by: Mark Brown <broonie@kernel.org>
Cc: stable@vger.kernel.org
---
 sound/soc/soc-ops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index c31e63b27193..dc0e7c8d31f3 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -879,6 +879,8 @@ int snd_soc_put_xr_sx(struct snd_kcontrol *kcontrol,
 	long val = ucontrol->value.integer.value[0];
 	unsigned int i;
 
+	if (val < mc->min || val > mc->max)
+		return -EINVAL;
 	if (invert)
 		val = max - val;
 	val &= mask;
-- 
2.30.2

Re: [PATCH v1 0/3] ASoC: Add bounds checking for written values

[Jaroslav Kysela]

On 24. 01. 22 16:32, Mark Brown wrote:
> This series adds verification that values written to registers are in
> bounds for controls since the core doesn't validate for us.

As discussed, those conditions should be optional to eventually catch the 
wrong applications. I don't see any benefit to report the range error back 
when there is value masking code already. The users will note when the 
unwanted values are written to the hardware, or not?

			Thanks,
				Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH v1 0/3] ASoC: Add bounds checking for written values

[Mark Brown]

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

On Mon, Jan 24, 2022 at 05:29:50PM +0100, Jaroslav Kysela wrote:
> On 24. 01. 22 16:32, Mark Brown wrote:

> > This series adds verification that values written to registers are in
> > bounds for controls since the core doesn't validate for us.

> As discussed, those conditions should be optional to eventually catch the
> wrong applications. I don't see any benefit to report the range error back
> when there is value masking code already. The users will note when the
> unwanted values are written to the hardware, or not?

In general I'd say that silent failures are harder to work with than
returning an error at the point where we notice that there's a problem,
assuming userspace is paying any attention to the error return at all of
course.  We certainly have quite a lot of existing put() methods that do
return errors and it's not like the application isn't invoking undefined
behaviour so I don't see a problem here.

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

Re: [PATCH v1 0/3] ASoC: Add bounds checking for written values

[Takashi Iwai]

On Mon, 24 Jan 2022 17:52:46 +0100,
Mark Brown wrote:
> 
> On Mon, Jan 24, 2022 at 05:29:50PM +0100, Jaroslav Kysela wrote:
> > On 24. 01. 22 16:32, Mark Brown wrote:
> 
> > > This series adds verification that values written to registers are in
> > > bounds for controls since the core doesn't validate for us.
> 
> > As discussed, those conditions should be optional to eventually catch the
> > wrong applications. I don't see any benefit to report the range error back
> > when there is value masking code already. The users will note when the
> > unwanted values are written to the hardware, or not?
> 
> In general I'd say that silent failures are harder to work with than
> returning an error at the point where we notice that there's a problem,
> assuming userspace is paying any attention to the error return at all of
> course.  We certainly have quite a lot of existing put() methods that do
> return errors and it's not like the application isn't invoking undefined
> behaviour so I don't see a problem here.

I find also it's more useful to have the proper checks in general.

Jaroslav, is you concern only about the compatibility of user-space?
Or anything else?  The compatibility is always certainly a slight
issue; if this breaks really something useful and actually working
stuff, we need to consider the workaround...


thanks,

Takashi

Re: [PATCH v1 0/3] ASoC: Add bounds checking for written values

[Jaroslav Kysela]

On 24. 01. 22 18:08, Takashi Iwai wrote:
> On Mon, 24 Jan 2022 17:52:46 +0100,
> Mark Brown wrote:
>>
>> On Mon, Jan 24, 2022 at 05:29:50PM +0100, Jaroslav Kysela wrote:
>>> On 24. 01. 22 16:32, Mark Brown wrote:
>>
>>>> This series adds verification that values written to registers are in
>>>> bounds for controls since the core doesn't validate for us.
>>
>>> As discussed, those conditions should be optional to eventually catch the
>>> wrong applications. I don't see any benefit to report the range error back
>>> when there is value masking code already. The users will note when the
>>> unwanted values are written to the hardware, or not?
>>
>> In general I'd say that silent failures are harder to work with than
>> returning an error at the point where we notice that there's a problem,
>> assuming userspace is paying any attention to the error return at all of
>> course.  We certainly have quite a lot of existing put() methods that do
>> return errors and it's not like the application isn't invoking undefined
>> behaviour so I don't see a problem here.
> 
> I find also it's more useful to have the proper checks in general.
> 
> Jaroslav, is you concern only about the compatibility of user-space?
> Or anything else?  The compatibility is always certainly a slight
> issue; if this breaks really something useful and actually working
> stuff, we need to consider the workaround...

My concern is only based on the fact that this code is normally not reachable. 
It only costs some CPU ticks and adds extra code to the drivers without any 
other benefit. The applications should not use out of range values and if they 
do, the behavior is not defined (the drivers should only avoid crashes).

The code seems to be added only to make things consistent for the test 
applications. I don't think that it's worth to care only for this reason. What 
is the goal for this code? The result with the proposed code will be - the SoC 
core has the extra validation conditions.

The user space can eventually add similar checks to detect the broken 
applications.

Basically, I think that those checks should be marked as debug and they should 
be optional like we do for CONFIG_SND_CTL_VALIDATION and enable only the fast 
path by default.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH v1 0/3] ASoC: Add bounds checking for written values

[Mark Brown]

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

On Mon, Jan 24, 2022 at 08:20:25PM +0100, Jaroslav Kysela wrote:
> On 24. 01. 22 18:08, Takashi Iwai wrote:

> > Jaroslav, is you concern only about the compatibility of user-space?
> > Or anything else?  The compatibility is always certainly a slight
> > issue; if this breaks really something useful and actually working
> > stuff, we need to consider the workaround...

> My concern is only based on the fact that this code is normally not
> reachable. It only costs some CPU ticks and adds extra code to the drivers
> without any other benefit. The applications should not use out of range
> values and if they do, the behavior is not defined (the drivers should only
> avoid crashes).

> The code seems to be added only to make things consistent for the test
> applications. I don't think that it's worth to care only for this reason.
> What is the goal for this code? The result with the proposed code will be -
> the SoC core has the extra validation conditions.

We need these checks all the time for the generic ASoC controls since
the values for the controls are stored in the underlying device's
register map so the out of range values will be written to the hardware,
pushing it out of specified use.  That's not a great idea in general and
in extreme cases could result in physical damage to the system.  The
biggest risk I see here is around speaker drivers since they deal with
high powers, even ignoring silicon requirements we also don't currently
enforce platform maximums that the machine drivers specify - that
feature was added after an inexperienced user burnt out the speakers in
their Chromebook since the speakers in the system were rated for much
lower powers than the CODEC was able to deliver.

> Basically, I think that those checks should be marked as debug and they
> should be optional like we do for CONFIG_SND_CTL_VALIDATION and enable only
> the fast path by default.

Note also that for everything using these helpers the underlying
register map will be regmap based and with the possible exception of
MMIO based regmaps the cost of writing out the new register value will
be overwhelmingly greater than that of the bounds checks.  It is
extremely hard to envision a scenario in which even a pathological
application would be able to observe a meaningful performance impact.

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

Re: [PATCH v1 0/3] ASoC: Add bounds checking for written values

[Jaroslav Kysela]

On 24. 01. 22 21:16, Mark Brown wrote:
> On Mon, Jan 24, 2022 at 08:20:25PM +0100, Jaroslav Kysela wrote:
>> On 24. 01. 22 18:08, Takashi Iwai wrote:
> 
>>> Jaroslav, is you concern only about the compatibility of user-space?
>>> Or anything else?  The compatibility is always certainly a slight
>>> issue; if this breaks really something useful and actually working
>>> stuff, we need to consider the workaround...
> 
>> My concern is only based on the fact that this code is normally not
>> reachable. It only costs some CPU ticks and adds extra code to the drivers
>> without any other benefit. The applications should not use out of range
>> values and if they do, the behavior is not defined (the drivers should only
>> avoid crashes).
> 
>> The code seems to be added only to make things consistent for the test
>> applications. I don't think that it's worth to care only for this reason.
>> What is the goal for this code? The result with the proposed code will be -
>> the SoC core has the extra validation conditions.
> 
> We need these checks all the time for the generic ASoC controls since
> the values for the controls are stored in the underlying device's
> register map so the out of range values will be written to the hardware,
> pushing it out of specified use.  That's not a great idea in general and
> in extreme cases could result in physical damage to the system.  The
> biggest risk I see here is around speaker drivers since they deal with
> high powers, even ignoring silicon requirements we also don't currently
> enforce platform maximums that the machine drivers specify - that
> feature was added after an inexperienced user burnt out the speakers in
> their Chromebook since the speakers in the system were rated for much
> lower powers than the CODEC was able to deliver.

It may be good to add this to the comment for two related patches. It's not 
obvious from the first glance. So one condition is a must.

>> Basically, I think that those checks should be marked as debug and they
>> should be optional like we do for CONFIG_SND_CTL_VALIDATION and enable only
>> the fast path by default.
> 
> Note also that for everything using these helpers the underlying
> register map will be regmap based and with the possible exception of
> MMIO based regmaps the cost of writing out the new register value will
> be overwhelmingly greater than that of the bounds checks.  It is
> extremely hard to envision a scenario in which even a pathological
> application would be able to observe a meaningful performance impact.

It's true that your patches are touching the universal code.

I stop complain for now. I just don't like to add chunks of the extra code 
just to resolve something which is not eventually a problem.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

Re: [PATCH v1 0/3] ASoC: Add bounds checking for written values

[Mark Brown]

On Mon, 24 Jan 2022 15:32:50 +0000, Mark Brown wrote:
> This series adds verification that values written to registers are in
> bounds for controls since the core doesn't validate for us.
> 
> Mark Brown (3):
>   ASoC: ops: Reject out of bounds values in snd_soc_put_volsw()
>   ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()
>   ASoC: ops: Reject out of bounds values in snd_soc_put_xr_sx()
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-linus

Thanks!

[1/3] ASoC: ops: Reject out of bounds values in snd_soc_put_volsw()
      commit: 817f7c9335ec01e0f5e8caffc4f1dcd5e458a4c0
[2/3] ASoC: ops: Reject out of bounds values in snd_soc_put_volsw_sx()
      commit: 4f1e50d6a9cf9c1b8c859d449b5031cacfa8404e
[3/3] ASoC: ops: Reject out of bounds values in snd_soc_put_xr_sx()
      commit: 4cf28e9ae6e2e11a044be1bcbcfa1b0d8675fe4d

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