Re: [PATCH 07/11] iio:health:afe4403 Fix timestamp alignment and prevent data leak.

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On 18 May 2020 01:09:47 BST, "Andrew F. Davis" <afd@ti.com> wrote:
>On 5/17/20 1:29 PM, jic23@kernel.org wrote:
>> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> 
>> One of a class of bugs pointed out by Lars in a recent review.
>> iio_push_to_buffers_with_timestamp assumes the buffer used is aligned
>> to the size of the timestamp (8 bytes).  This is not guaranteed in
>> this driver which uses a 32 byte array of smaller elements on the
>stack.
>> As Lars also noted this anti pattern can involve a leak of data to
>> userspace and that indeed can happen here.  We close both issues by
>> moving to a suitable structure in the iio_priv() data with alignment
>> explicitly requested.  This data is allocated with kzalloc so no
>> data can leak appart from previous readings.
>> 
>> Fixes: eec96d1e2d31 ("iio: health: Add driver for the TI AFE4403
>heart monitor")
>> Reported-by: Lars-Peter Clausen <lars@metafoo.de>
>> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> Cc: Andrew F. Davis <afd@ti.com>
>> ---
>>  drivers/iio/health/afe4403.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/iio/health/afe4403.c
>b/drivers/iio/health/afe4403.c
>> index e9f87e42ff4f..a3507624b30f 100644
>> --- a/drivers/iio/health/afe4403.c
>> +++ b/drivers/iio/health/afe4403.c
>> @@ -65,6 +65,7 @@ static const struct reg_field afe4403_reg_fields[]
>= {
>>   * @regulator: Pointer to the regulator for the IC
>>   * @trig: IIO trigger for this device
>>   * @irq: ADC_RDY line interrupt number
>> + * @buffer: Used to construct data layout to push into IIO buffer.
>>   */
>>  struct afe4403_data {
>>  	struct device *dev;
>> @@ -74,6 +75,8 @@ struct afe4403_data {
>>  	struct regulator *regulator;
>>  	struct iio_trigger *trig;
>>  	int irq;
>> +	/* Ensure suitable alignment for timestamp */
>> +	s32 buffer[8] __aligned(8);
>
>
>One of those fancy structs with the timestamp specified would be nice
>here like the other patches. IIRC we have 6 s32 channels, plus a s64
>ts.

I think we may only have some of those channels enabled.  So ts may be it several
locations in the buffer. 

Hence we could use the structure approach but it might give a false sense
of what is going on. 

J
>
>Other than that everything looks good.
>
>Andrew
>
>
>>  };
>>  
>>  enum afe4403_chan_id {
>> @@ -309,7 +312,6 @@ static irqreturn_t afe4403_trigger_handler(int
>irq, void *private)
>>  	struct iio_dev *indio_dev = pf->indio_dev;
>>  	struct afe4403_data *afe = iio_priv(indio_dev);
>>  	int ret, bit, i = 0;
>> -	s32 buffer[8];
>>  	u8 tx[4] = {AFE440X_CONTROL0, 0x0, 0x0, AFE440X_CONTROL0_READ};
>>  	u8 rx[3];
>>  
>> @@ -326,7 +328,7 @@ static irqreturn_t afe4403_trigger_handler(int
>irq, void *private)
>>  		if (ret)
>>  			goto err;
>>  
>> -		buffer[i++] = get_unaligned_be24(&rx[0]);
>> +		afe->buffer[i++] = get_unaligned_be24(&rx[0]);
>>  	}
>>  
>>  	/* Disable reading from the device */
>> @@ -335,7 +337,8 @@ static irqreturn_t afe4403_trigger_handler(int
>irq, void *private)
>>  	if (ret)
>>  		goto err;
>>  
>> -	iio_push_to_buffers_with_timestamp(indio_dev, buffer,
>pf->timestamp);
>> +	iio_push_to_buffers_with_timestamp(indio_dev, afe->buffer,
>> +					   pf->timestamp);
>>  err:
>>  	iio_trigger_notify_done(indio_dev->trig);
>>  
>> 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.