[PATCH 1/2] ASoC: ops: fix signedness bug in snd_soc_put_volsw()

[PATCH 1/2] ASoC: ops: fix signedness bug in snd_soc_put_volsw()

[Dan Carpenter]

The "val" and "val2" variables need to signed for the checking to work
as intended.

Fixes: 817f7c9335ec ("ASoC: ops: Reject out of bounds values in snd_soc_put_volsw()")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 sound/soc/soc-ops.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/sound/soc/soc-ops.c b/sound/soc/soc-ops.c
index dc0e7c8d31f3..0091fa96eb48 100644
--- a/sound/soc/soc-ops.c
+++ b/sound/soc/soc-ops.c
@@ -310,8 +310,9 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol,
 	unsigned int invert = mc->invert;
 	int err;
 	bool type_2r = false;
-	unsigned int val2 = 0;
-	unsigned int val, val_mask;
+	unsigned int val_mask;
+	int val2 = 0;
+	int val;
 
 	if (sign_bit)
 		mask = BIT(sign_bit + 1) - 1;
-- 
2.20.1

Re: [PATCH 1/2] ASoC: ops: fix signedness bug in snd_soc_put_volsw()

[Mark Brown]

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

On Fri, Jan 28, 2022 at 02:20:07PM +0300, Dan Carpenter wrote:
> The "val" and "val2" variables need to signed for the checking to work
> as intended.

This means that the helpers won't support controls that use the top bit
of a 32 bit register.

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

Re: [PATCH 1/2] ASoC: ops: fix signedness bug in snd_soc_put_volsw()

[Dan Carpenter]

On Fri, Jan 28, 2022 at 12:42:04PM +0000, Mark Brown wrote:
> On Fri, Jan 28, 2022 at 02:20:07PM +0300, Dan Carpenter wrote:
> > The "val" and "val2" variables need to signed for the checking to work
> > as intended.
> 
> This means that the helpers won't support controls that use the top bit
> of a 32 bit register.

Fine, I can delete the checks for negative instead (I'm surprised you
haven't already received a dozen bot emails about this).

I haven't been able to figure out where mc->min/max are set.  In
snd_soc_get_xr_sx() it checks whether "mc->min" is negative.

	if (min < 0 && val > max)
		val |= ~mask;

Is that a bug?  If mc->min is negative the math gets tricky.

regards,
dan carpenter

Re: [PATCH 1/2] ASoC: ops: fix signedness bug in snd_soc_put_volsw()

[Mark Brown]

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

On Fri, Jan 28, 2022 at 04:31:47PM +0300, Dan Carpenter wrote:
> On Fri, Jan 28, 2022 at 12:42:04PM +0000, Mark Brown wrote:

> > This means that the helpers won't support controls that use the top bit
> > of a 32 bit register.

> Fine, I can delete the checks for negative instead (I'm surprised you
> haven't already received a dozen bot emails about this).

No, we need the checks for negatives since userspace supplies a signed
value.  The check needs to be done on the value in the input structure
before we pull it out for mangling.  I probably got bot emails but
frankly these days almost all of them are some combination of barely
legible and misdirected and there's plenty of people who like to fix
these things if they're real.

> I haven't been able to figure out where mc->min/max are set.  In

They're the big tables of static assignments via macros in the drivers. 

> snd_soc_get_xr_sx() it checks whether "mc->min" is negative.

> 	if (min < 0 && val > max)
> 		val |= ~mask;

> Is that a bug?  If mc->min is negative the math gets tricky.

No, it *is* valid if weird to have negative values for some controls.

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

Re: [PATCH 1/2] ASoC: ops: fix signedness bug in snd_soc_put_volsw()

[Dan Carpenter]

On Fri, Jan 28, 2022 at 01:48:44PM +0000, Mark Brown wrote:
> On Fri, Jan 28, 2022 at 04:31:47PM +0300, Dan Carpenter wrote:
> > On Fri, Jan 28, 2022 at 12:42:04PM +0000, Mark Brown wrote:
> 
> > > This means that the helpers won't support controls that use the top bit
> > > of a 32 bit register.
> 
> > Fine, I can delete the checks for negative instead (I'm surprised you
> > haven't already received a dozen bot emails about this).
> 
> No, we need the checks for negatives since userspace supplies a signed
> value.  The check needs to be done on the value in the input structure
> before we pull it out for mangling.  I probably got bot emails but
> frankly these days almost all of them are some combination of barely
> legible and misdirected and there's plenty of people who like to fix
> these things if they're real.
> 

That's a bit trickier than I initially imagined so I'm going to leave
this for smarter people than me...

regards,
dan carpenter