Re: [PATCH v2 01/15] iio: adc: ad4134: Sign extend sample data

[Jonathan Cameron]

> Sign extend ADC sample data for correct delivery of negative ADC input
> difference.
> 
> Fixes: e0bc6d7e2584 ("iio: adc: Initial support for AD4134")
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@xxxxxxxxxx>
Hi Marcelo, Sashiko made a comment and I have no idea if it is
right or not!

>
> diff --git a/drivers/iio/adc/ad4134.c b/drivers/iio/adc/ad4134.c
> index 0490218bb0e9..7d40c0521bb3 100644
> --- a/drivers/iio/adc/ad4134.c
> +++ b/drivers/iio/adc/ad4134.c
> @@ -172,6 +172,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  			    unsigned int *val)
>  {
>  	unsigned int i;
> +	u32 sample;
>  	int ret;
>  
>  	/*
> @@ -196,8 +197,9 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,
>  		 * Clock out data from all channels to avoid that.
>  		 */
>  		if (i == AD4134_VREG_CH(reg))
> -			*val = get_unaligned_be24(st->rx_buf);
> +			sample = get_unaligned_be24(st->rx_buf);
>  	}
> +	*val = sign_extend32(sample, AD4134_CHAN_PRECISION_BITS - 1);

Sashiko.
[Severity: High]
Does this code leak uninitialized stack memory to userspace?

If a privileged user reads register 0x54 via the IIO debugfs
direct_reg_access file, ad4134_data_read() is invoked with reg=0x54. This is
possible because of a pre-existing off-by-one error in the regmap
configuration (where AD4134_CH_VREG(AD4134_NUM_CHANNELS) evaluates to 0x54)
that allows read access to this virtual register.

Because the loop index 'i' only iterates up to 3, the condition
'i == AD4134_VREG_CH(0x54)' is never met and 'sample' is left uninitialized.
The newly introduced code then unconditionally sign-extends this uninitialized
memory and writes it back to '*val', which is then returned to the user.
-
Smells potentially valid givne previously val was not set unless
we had a match and now it is always set.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>