| Message ID | 20220128112007.GA24806@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ASoC: ops: fix signedness bug in snd_soc_put_volsw() | expand |
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.
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
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.
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
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;
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(-)