| Message ID | 20200517173000.220819-8-jic23@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IIO: 1st set of timestamp alignment fixes. | expand |
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. 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); > >
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); >> >>
On 5/18/20 12:06 PM, Jonathan Cameron wrote: > > > 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. > Might have been better to have the ts at the beginning, could have also helped with alignment for when an odd number of channels are enabled. > Hence we could use the structure approach but it might give a false sense > of what is going on. > That's true, it can always be cleaned later then. Andrew > 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); >>> >>> >
On 21 May 2020 14:11:10 BST, "Andrew F. Davis" <afd@ti.com> wrote: > > >On 5/18/20 12:06 PM, Jonathan Cameron wrote: >> >> >> 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. >> > >Might have been better to have the ts at the beginning, could have also >helped with alignment for when an odd number of channels are enabled. Perhaps but for many use cases we don't turn timestamps on and we would need to pad anyway for alignment of the fifo. > >> Hence we could use the structure approach but it might give a false >sense >> of what is going on. >> > >That's true, it can always be cleaned later then. Agreed. This is definitely something we can revisit sometime in the future. J > >Andrew > >> 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); >>>> >>>> >>
On Thu, 21 May 2020 15:44:43 +0100 Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > On 21 May 2020 14:11:10 BST, "Andrew F. Davis" <afd@ti.com> wrote: > > > > > >On 5/18/20 12:06 PM, Jonathan Cameron wrote: > >> > >> > >> 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. > >> > > > >Might have been better to have the ts at the beginning, could have also > >helped with alignment for when an odd number of channels are enabled. > > Perhaps but for many use cases we don't turn timestamps on and we would need to pad > anyway for alignment of the fifo. > > > > >> Hence we could use the structure approach but it might give a false > >sense > >> of what is going on. > >> > > > >That's true, it can always be cleaned later then. > > Agreed. This is definitely something we can revisit sometime in the future. Formal Ack on applying the two afe patches? Thanks, Jonathan > > J > > > >Andrew > > > >> 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); > >>>> > >>>> > >> >
On 5/21/20 2:45 PM, Jonathan Cameron wrote: > On Thu, 21 May 2020 15:44:43 +0100 > Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > >> On 21 May 2020 14:11:10 BST, "Andrew F. Davis" <afd@ti.com> wrote: >>> >>> >>> On 5/18/20 12:06 PM, Jonathan Cameron wrote: >>>> >>>> >>>> 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. >>>> >>> >>> Might have been better to have the ts at the beginning, could have also >>> helped with alignment for when an odd number of channels are enabled. >> >> Perhaps but for many use cases we don't turn timestamps on and we would need to pad >> anyway for alignment of the fifo. >> >>> >>>> Hence we could use the structure approach but it might give a false >>> sense >>>> of what is going on. >>>> >>> >>> That's true, it can always be cleaned later then. >> >> Agreed. This is definitely something we can revisit sometime in the future. > > Formal Ack on applying the two afe patches? > Acked-by: Andrew F. Davis <afd@ti.com> > Thanks, > > Jonathan >> >> J >>> >>> Andrew >>> >>>> 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); >>>>>> >>>>>> >>>> >> >
On Thu, 21 May 2020 15:05:37 -0400 "Andrew F. Davis" <afd@ti.com> wrote: > On 5/21/20 2:45 PM, Jonathan Cameron wrote: > > On Thu, 21 May 2020 15:44:43 +0100 > > Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote: > > > >> On 21 May 2020 14:11:10 BST, "Andrew F. Davis" <afd@ti.com> wrote: > >>> > >>> > >>> On 5/18/20 12:06 PM, Jonathan Cameron wrote: > >>>> > >>>> > >>>> 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. > >>>> > >>> > >>> Might have been better to have the ts at the beginning, could have also > >>> helped with alignment for when an odd number of channels are enabled. > >> > >> Perhaps but for many use cases we don't turn timestamps on and we would need to pad > >> anyway for alignment of the fifo. > >> > >>> > >>>> Hence we could use the structure approach but it might give a false > >>> sense > >>>> of what is going on. > >>>> > >>> > >>> That's true, it can always be cleaned later then. > >> > >> Agreed. This is definitely something we can revisit sometime in the future. > > > > Formal Ack on applying the two afe patches? > > > > Acked-by: Andrew F. Davis <afd@ti.com> Thanks. Applied to the fixes-togreg branch of iio.git and marked for stable. These won't go anywhere now until after the merge window. Thanks, Jonathan > > > Thanks, > > > > Jonathan > >> > >> J > >>> > >>> Andrew > >>> > >>>> 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); > >>>>>> > >>>>>> > >>>> > >> > >
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); }; 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);