[PATCH] iio: adc: at91-sama5d2_adc: fix up casting in at91_adc_read_info_raw()

[PATCH] iio: adc: at91-sama5d2_adc: fix up casting in at91_adc_read_info_raw()

[Dan Carpenter]

This code is problematic because we're supposed to be writing an int but
we instead write to only the high 16 bits.  This doesn't work on big
endian systems, and there is a potential that the bottom 16 bits are
used without being initialized.

Fixes: 23ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index e02f7d1c86bc..d5ea84cf6460 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1296,6 +1296,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 {
 	struct at91_adc_state *st = iio_priv(indio_dev);
 	u32 cor = 0;
+	u16 tmp_val;
 	int ret;
 
 	/*
@@ -1309,7 +1310,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		mutex_lock(&st->lock);
 
 		ret = at91_adc_read_position(st, chan->channel,
-					     (u16 *)val);
+					     &tmp_val);
+		*val = tmp_val;
 		mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 
@@ -1322,7 +1324,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
 		mutex_lock(&st->lock);
 
 		ret = at91_adc_read_pressure(st, chan->channel,
-					     (u16 *)val);
+					     &tmp_val);
+		*val = tmp_val;
 		mutex_unlock(&st->lock);
 		iio_device_release_direct_mode(indio_dev);
 

Re: [PATCH] iio: adc: at91-sama5d2_adc: fix up casting in at91_adc_read_info_raw()

[Eugen Hristev]

On 09.07.2018 14:06, Dan Carpenter wrote:
> This code is problematic because we're supposed to be writing an int but
> we instead write to only the high 16 bits.  This doesn't work on big
> endian systems, and there is a potential that the bottom 16 bits are
> used without being initialized.

Hi Dan,

Thanks for the patch.
Please correct me if I'm wrong, the caller of this function should mask 
out the unused bits w.r.t. the channel spec ?

Indeed there may be an issue if we actually write the data to the wrong 
16 bit part of the 32 bit integer.

Would be safer to check for the endianess and write the proper part of 
the int ? (macros that do the magic for us - cpu_to_le etc.), or we rely 
on the compiler to do it for us as it looks in your code ?

Another option is to pass the int directly and do the ugly task inside 
the read_position/pressure functions, I am not sure which one looks better

Thanks,
Eugen

> 
> Fixes: 23ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index e02f7d1c86bc..d5ea84cf6460 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1296,6 +1296,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>   {
>   	struct at91_adc_state *st = iio_priv(indio_dev);
>   	u32 cor = 0;
> +	u16 tmp_val;
>   	int ret;
>   
>   	/*
> @@ -1309,7 +1310,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>   		mutex_lock(&st->lock);
>   
>   		ret = at91_adc_read_position(st, chan->channel,
> -					     (u16 *)val);
> +					     &tmp_val);
> +		*val = tmp_val;
>   		mutex_unlock(&st->lock);
>   		iio_device_release_direct_mode(indio_dev);
>   
> @@ -1322,7 +1324,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>   		mutex_lock(&st->lock);
>   
>   		ret = at91_adc_read_pressure(st, chan->channel,
> -					     (u16 *)val);
> +					     &tmp_val);
> +		*val = tmp_val;
>   		mutex_unlock(&st->lock);
>   		iio_device_release_direct_mode(indio_dev);
>   
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] iio: adc: at91-sama5d2_adc: fix up casting in at91_adc_read_info_raw()

[Dan Carpenter]

On Mon, Jul 09, 2018 at 02:22:40PM +0300, Eugen Hristev wrote:
> 
> 
> On 09.07.2018 14:06, Dan Carpenter wrote:
> > This code is problematic because we're supposed to be writing an int but
> > we instead write to only the high 16 bits.  This doesn't work on big
> > endian systems, and there is a potential that the bottom 16 bits are
> > used without being initialized.
> 
> Hi Dan,
> 
> Thanks for the patch.
> Please correct me if I'm wrong, the caller of this function should mask out
> the unused bits w.r.t. the channel spec ?
> 
> Indeed there may be an issue if we actually write the data to the wrong 16
> bit part of the 32 bit integer.
> 
> Would be safer to check for the endianess and write the proper part of the
> int ? (macros that do the magic for us - cpu_to_le etc.), or we rely on the
> compiler to do it for us as it looks in your code ?
> 
> Another option is to pass the int directly and do the ugly task inside the
> read_position/pressure functions, I am not sure which one looks better
> 

To be honest, I'm just doing static analysis.  I'm not very familiar
with the subsystem and I don't know the answers to your questions.

The code as it's written now doesn't make sense.  I looked at code like
ntc_adc_iio_read() where it's called like so:

	int raw, uv, ret;

	ret = iio_read_channel_raw(channel, &raw);

If we only write to the high 16 bits then the low 16 bits of "raw" are
uninitalized.

regards,
dan carpenter

Re: [PATCH] iio: adc: at91-sama5d2_adc: fix up casting in at91_adc_read_info_raw()

[Jonathan Cameron]

On Mon, 9 Jul 2018 14:41:07 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Mon, Jul 09, 2018 at 02:22:40PM +0300, Eugen Hristev wrote:
> > 
> > 
> > On 09.07.2018 14:06, Dan Carpenter wrote:  
> > > This code is problematic because we're supposed to be writing an int but
> > > we instead write to only the high 16 bits.  This doesn't work on big
> > > endian systems, and there is a potential that the bottom 16 bits are
> > > used without being initialized.  
> > 
> > Hi Dan,
> > 
> > Thanks for the patch.
> > Please correct me if I'm wrong, the caller of this function should mask out
> > the unused bits w.r.t. the channel spec ?

Nope, the chan spec shift stuff is only for buffered reads, they are not used
in the read_raw paths.  We could in theory, but we don't.  Given those raw
reads do a lot more than just read values, (scales etc) it would be really
odd to do masking for the raw values, but nothing else. It's expected that
a driver will deal with that itself.  If nothing else, lots of drivers don't
have that section of the chan spec filled in because it would just make them
more complex.

When you are using the buffered interfaces it is acceptable to not mask
out other bits that are definitely coming from the hardware, however we should
still mask out any that are due to not initializing local variables as otherwise
we leak kernel data.

> > 
> > Indeed there may be an issue if we actually write the data to the wrong 16
> > bit part of the 32 bit integer.
> > 
> > Would be safer to check for the endianess and write the proper part of the
> > int ? (macros that do the magic for us - cpu_to_le etc.), or we rely on the
> > compiler to do it for us as it looks in your code ?
> > 
> > Another option is to pass the int directly and do the ugly task inside the
> > read_position/pressure functions, I am not sure which one looks better
> >   
> 
> To be honest, I'm just doing static analysis.  I'm not very familiar
> with the subsystem and I don't know the answers to your questions.
> 
> The code as it's written now doesn't make sense.  I looked at code like
> ntc_adc_iio_read() where it's called like so:
> 
> 	int raw, uv, ret;
> 
> 	ret = iio_read_channel_raw(channel, &raw);
> 
> If we only write to the high 16 bits then the low 16 bits of "raw" are
> uninitalized.
Superficially Dan's patch looks right to me, but I would like some
testing to make sure nothing odd is taking advantage of the previous
'unusual :)' code.

Jonathan

> 
> regards,
> dan carpenter
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iio: adc: at91-sama5d2_adc: fix up casting in at91_adc_read_info_raw()

[Eugen Hristev]

On 15.07.2018 11:15, Jonathan Cameron wrote:
> On Mon, 9 Jul 2018 14:41:07 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
>> On Mon, Jul 09, 2018 at 02:22:40PM +0300, Eugen Hristev wrote:
>>>
>>>
>>> On 09.07.2018 14:06, Dan Carpenter wrote:
>>>> This code is problematic because we're supposed to be writing an int but
>>>> we instead write to only the high 16 bits.  This doesn't work on big
>>>> endian systems, and there is a potential that the bottom 16 bits are
>>>> used without being initialized.
>>>
>>> Hi Dan,
>>>
>>> Thanks for the patch.
>>> Please correct me if I'm wrong, the caller of this function should mask out
>>> the unused bits w.r.t. the channel spec ?
> 
> Nope, the chan spec shift stuff is only for buffered reads, they are not used
> in the read_raw paths.  We could in theory, but we don't.  Given those raw
> reads do a lot more than just read values, (scales etc) it would be really
> odd to do masking for the raw values, but nothing else. It's expected that
> a driver will deal with that itself.  If nothing else, lots of drivers don't
> have that section of the chan spec filled in because it would just make them
> more complex.
> 
> When you are using the buffered interfaces it is acceptable to not mask
> out other bits that are definitely coming from the hardware, however we should
> still mask out any that are due to not initializing local variables as otherwise
> we leak kernel data.
> 
>>>
>>> Indeed there may be an issue if we actually write the data to the wrong 16
>>> bit part of the 32 bit integer.
>>>
>>> Would be safer to check for the endianess and write the proper part of the
>>> int ? (macros that do the magic for us - cpu_to_le etc.), or we rely on the
>>> compiler to do it for us as it looks in your code ?
>>>
>>> Another option is to pass the int directly and do the ugly task inside the
>>> read_position/pressure functions, I am not sure which one looks better
>>>    
>>
>> To be honest, I'm just doing static analysis.  I'm not very familiar
>> with the subsystem and I don't know the answers to your questions.
>>
>> The code as it's written now doesn't make sense.  I looked at code like
>> ntc_adc_iio_read() where it's called like so:
>>
>> 	int raw, uv, ret;
>>
>> 	ret = iio_read_channel_raw(channel, &raw);
>>
>> If we only write to the high 16 bits then the low 16 bits of "raw" are
>> uninitalized.
> Superficially Dan's patch looks right to me, but I would like some
> testing to make sure nothing odd is taking advantage of the previous
> 'unusual :)' code.

Tested-by: Eugen Hristev <eugen.hristev@microchip.com>

> 
> Jonathan
> 
>>
>> regards,
>> dan carpenter
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH] iio: adc: at91-sama5d2_adc: fix up casting in at91_adc_read_info_raw()

[Ludovic Desroches]

On Mon, Jul 09, 2018 at 02:06:59PM +0300, Dan Carpenter wrote:
> This code is problematic because we're supposed to be writing an int but
> we instead write to only the high 16 bits.  This doesn't work on big
> endian systems, and there is a potential that the bottom 16 bits are
> used without being initialized.
> 
> Fixes: 23ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 

This patch sounds good and as it has been tested by Eugen:
Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>

Thanks Dan and Eugen.

> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index e02f7d1c86bc..d5ea84cf6460 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1296,6 +1296,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  {
>  	struct at91_adc_state *st = iio_priv(indio_dev);
>  	u32 cor = 0;
> +	u16 tmp_val;
>  	int ret;
>  
>  	/*
> @@ -1309,7 +1310,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		mutex_lock(&st->lock);
>  
>  		ret = at91_adc_read_position(st, chan->channel,
> -					     (u16 *)val);
> +					     &tmp_val);
> +		*val = tmp_val;
>  		mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
> @@ -1322,7 +1324,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
>  		mutex_lock(&st->lock);
>  
>  		ret = at91_adc_read_pressure(st, chan->channel,
> -					     (u16 *)val);
> +					     &tmp_val);
> +		*val = tmp_val;
>  		mutex_unlock(&st->lock);
>  		iio_device_release_direct_mode(indio_dev);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] iio: adc: at91-sama5d2_adc: fix up casting in at91_adc_read_info_raw()

[Jonathan Cameron]

On Tue, 17 Jul 2018 10:12:00 +0200
Ludovic Desroches <ludovic.desroches@microchip.com> wrote:

> On Mon, Jul 09, 2018 at 02:06:59PM +0300, Dan Carpenter wrote:
> > This code is problematic because we're supposed to be writing an int but
> > we instead write to only the high 16 bits.  This doesn't work on big
> > endian systems, and there is a potential that the bottom 16 bits are
> > used without being initialized.
> > 
> > Fixes: 23ec2774f1cc ("iio: adc: at91-sama5d2_adc: add support for position and pressure channels")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >   
> 
> This patch sounds good and as it has been tested by Eugen:
> Acked-by: Ludovic Desroches <ludovic.desroches@microchip.com>
> 
> Thanks Dan and Eugen.

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it.

Thanks

Jonathan

> 
> > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> > index e02f7d1c86bc..d5ea84cf6460 100644
> > --- a/drivers/iio/adc/at91-sama5d2_adc.c
> > +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> > @@ -1296,6 +1296,7 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >  {
> >  	struct at91_adc_state *st = iio_priv(indio_dev);
> >  	u32 cor = 0;
> > +	u16 tmp_val;
> >  	int ret;
> >  
> >  	/*
> > @@ -1309,7 +1310,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >  		mutex_lock(&st->lock);
> >  
> >  		ret = at91_adc_read_position(st, chan->channel,
> > -					     (u16 *)val);
> > +					     &tmp_val);
> > +		*val = tmp_val;
> >  		mutex_unlock(&st->lock);
> >  		iio_device_release_direct_mode(indio_dev);
> >  
> > @@ -1322,7 +1324,8 @@ static int at91_adc_read_info_raw(struct iio_dev *indio_dev,
> >  		mutex_lock(&st->lock);
> >  
> >  		ret = at91_adc_read_pressure(st, chan->channel,
> > -					     (u16 *)val);
> > +					     &tmp_val);
> > +		*val = tmp_val;
> >  		mutex_unlock(&st->lock);
> >  		iio_device_release_direct_mode(indio_dev);
> >  
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html