When noevents is true and small buffer is used the allocated memory for
holding the data may be smaller than the hard-coded 64 bytes. This can
cause the iio_generic_buffer to crash.
Following was recorded on beagle bone black with v6.0 kernel and the
digit fix patch:
https://lore.kernel.org/all/[email protected]/
using valgrind;
==339== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==339== Command: /iio_generic_buffer -n kx022-accel -T0 -e -l 10 -a -w 2000000
==339== Parent PID: 307
==339==
==339== Syscall param read(buf) points to unaddressable byte(s)
==339== at 0x496BFA4: read (read.c:26)
==339== by 0x11699: main (iio_generic_buffer.c:724)
==339== Address 0x4ab3518 is 0 bytes after a block of size 160 alloc'd
==339== at 0x4864B70: malloc (vg_replace_malloc.c:381)
==339== by 0x115BB: main (iio_generic_buffer.c:677)
Fix this by always using the same size for reading as was used for
data storage allocation.
Signed-off-by: Matti Vaittinen <[email protected]>
---
This patch has been only tested with my kx022a sensor driver. Driver may
have some culprits(s) and my understanding regarding IIO and these tools
is limited so perhaps the hard-coded size of 64 bytes has perfectly
legitimate reason - in which case I would appreciate to hear the
reasoning so I could seek the problem from my driver. Also, I didn't add
the fixes-tag as I don't really know which commit has caused the problem
- as I am not 100% sure what the problem actually is and if I am just
fixing a symptom here.
---
tools/iio/iio_generic_buffer.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
index 2491c54a5e4f..f8deae4e26a1 100644
--- a/tools/iio/iio_generic_buffer.c
+++ b/tools/iio/iio_generic_buffer.c
@@ -715,12 +715,12 @@ int main(int argc, char **argv)
continue;
}
- toread = buf_len;
} else {
usleep(timedelay);
- toread = 64;
}
+ toread = buf_len;
+
read_size = read(buf_fd, data, toread * scan_size);
if (read_size < 0) {
if (errno == EAGAIN) {
base-commit: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f
--
2.37.3
--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~
Simon says - in Latin please.
~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~
Thanks to Simon Glass for the translation =]
On Fri, 14 Oct 2022 10:15:19 +0300
Matti Vaittinen <[email protected]> wrote:
> When noevents is true and small buffer is used the allocated memory for
> holding the data may be smaller than the hard-coded 64 bytes. This can
> cause the iio_generic_buffer to crash.
>
> Following was recorded on beagle bone black with v6.0 kernel and the
> digit fix patch:
> https://lore.kernel.org/all/[email protected]/
> using valgrind;
>
> ==339== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
> ==339== Command: /iio_generic_buffer -n kx022-accel -T0 -e -l 10 -a -w 2000000
> ==339== Parent PID: 307
> ==339==
> ==339== Syscall param read(buf) points to unaddressable byte(s)
> ==339== at 0x496BFA4: read (read.c:26)
> ==339== by 0x11699: main (iio_generic_buffer.c:724)
> ==339== Address 0x4ab3518 is 0 bytes after a block of size 160 alloc'd
> ==339== at 0x4864B70: malloc (vg_replace_malloc.c:381)
> ==339== by 0x115BB: main (iio_generic_buffer.c:677)
>
> Fix this by always using the same size for reading as was used for
> data storage allocation.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
>
Huh. I have no idea why that value is 64... And git blame says I wrote it
over 10 years ago :)
Patch looks fine to me, but given I don't understand the logic of the existing
code either I'll leave it on list for a little longer before picking it up.
> ---
>
> This patch has been only tested with my kx022a sensor driver. Driver may
> have some culprits(s) and my understanding regarding IIO and these tools
> is limited so perhaps the hard-coded size of 64 bytes has perfectly
> legitimate reason - in which case I would appreciate to hear the
> reasoning so I could seek the problem from my driver. Also, I didn't add
> the fixes-tag as I don't really know which commit has caused the problem
> - as I am not 100% sure what the problem actually is and if I am just
> fixing a symptom here.
> ---
> tools/iio/iio_generic_buffer.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> index 2491c54a5e4f..f8deae4e26a1 100644
> --- a/tools/iio/iio_generic_buffer.c
> +++ b/tools/iio/iio_generic_buffer.c
> @@ -715,12 +715,12 @@ int main(int argc, char **argv)
> continue;
> }
>
> - toread = buf_len;
> } else {
> usleep(timedelay);
> - toread = 64;
> }
>
> + toread = buf_len;
> +
> read_size = read(buf_fd, data, toread * scan_size);
> if (read_size < 0) {
> if (errno == EAGAIN) {
>
> base-commit: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f
On Sat, 15 Oct 2022 17:30:14 +0100
Jonathan Cameron <[email protected]> wrote:
> On Fri, 14 Oct 2022 10:15:19 +0300
> Matti Vaittinen <[email protected]> wrote:
>
> > When noevents is true and small buffer is used the allocated memory for
> > holding the data may be smaller than the hard-coded 64 bytes. This can
> > cause the iio_generic_buffer to crash.
> >
> > Following was recorded on beagle bone black with v6.0 kernel and the
> > digit fix patch:
> > https://lore.kernel.org/all/[email protected]/
> > using valgrind;
> >
> > ==339== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
> > ==339== Command: /iio_generic_buffer -n kx022-accel -T0 -e -l 10 -a -w 2000000
> > ==339== Parent PID: 307
> > ==339==
> > ==339== Syscall param read(buf) points to unaddressable byte(s)
> > ==339== at 0x496BFA4: read (read.c:26)
> > ==339== by 0x11699: main (iio_generic_buffer.c:724)
> > ==339== Address 0x4ab3518 is 0 bytes after a block of size 160 alloc'd
> > ==339== at 0x4864B70: malloc (vg_replace_malloc.c:381)
> > ==339== by 0x115BB: main (iio_generic_buffer.c:677)
> >
> > Fix this by always using the same size for reading as was used for
> > data storage allocation.
> >
> > Signed-off-by: Matti Vaittinen <[email protected]>
> >
>
> Huh. I have no idea why that value is 64... And git blame says I wrote it
> over 10 years ago :)
>
> Patch looks fine to me, but given I don't understand the logic of the existing
> code either I'll leave it on list for a little longer before picking it up.
Guess no one else read this or knows the answer if they did ;)
Applied to the fixes-togreg branch of iio.git
Thanks,
Jonathan
>
>
> > ---
> >
> > This patch has been only tested with my kx022a sensor driver. Driver may
> > have some culprits(s) and my understanding regarding IIO and these tools
> > is limited so perhaps the hard-coded size of 64 bytes has perfectly
> > legitimate reason - in which case I would appreciate to hear the
> > reasoning so I could seek the problem from my driver. Also, I didn't add
> > the fixes-tag as I don't really know which commit has caused the problem
> > - as I am not 100% sure what the problem actually is and if I am just
> > fixing a symptom here.
> > ---
> > tools/iio/iio_generic_buffer.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/iio/iio_generic_buffer.c b/tools/iio/iio_generic_buffer.c
> > index 2491c54a5e4f..f8deae4e26a1 100644
> > --- a/tools/iio/iio_generic_buffer.c
> > +++ b/tools/iio/iio_generic_buffer.c
> > @@ -715,12 +715,12 @@ int main(int argc, char **argv)
> > continue;
> > }
> >
> > - toread = buf_len;
> > } else {
> > usleep(timedelay);
> > - toread = 64;
> > }
> >
> > + toread = buf_len;
> > +
> > read_size = read(buf_fd, data, toread * scan_size);
> > if (read_size < 0) {
> > if (errno == EAGAIN) {
> >
> > base-commit: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f
>