Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 29 Oct 2022 16:42:32 +0100
From:   Jonathan Cameron <jic23@kernel.org>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] =?UTF-8?B?wqd0b29sczo=?= iio: iio_generic_buffer: Fix
 read size
Message-ID: <20221029164232.06582144@jic23-huawei>
In-Reply-To: <20221015173014.0fb92829@jic23-huawei>
References: <Y0kMh0t5qUXJw3nQ@dc75zzyyyyyyyyyyyyycy-3.rev.dnainternet.fi>
        <20221015173014.0fb92829@jic23-huawei>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sat, 15 Oct 2022 17:30:14 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 14 Oct 2022 10:15:19 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> 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/Y0f+tKCz+ZAIoroQ@dc75zzyyyyyyyyyyyyycy-3.rev.dnainternet.fi/
> > 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 <mazziesaccount@gmail.com>
> >   
> 
> 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  
>