Re: [PATCH] staging:iio:kfifo_buf: Fix potential buffer overflow in iio_read_first_n_kfifo

[Jonathan Cameron <jic23@kernel.org>]

On 12/07/2011 02:43 PM, Lars-Peter Clausen wrote:
> n is the number of bytes to read, not the number of samples. So if there is
> enough data available we will write to the userspace buffer beyond its bounds.
> Fix this by copying n bytes maximum. Also round n down to the next multiple of
> the sample size, so we will only read complete samples. If the buffer is too
> small to hold at least one sample return -EINVAL.
> 
Dratt. You are quite right.  The documentation for read_first_n is
rather cryptic.  It was a while ago, but I think the plan was that it
would be a call to read n scans though clearly it is not in either the
users or ring_sw.   I guess we might allow partial reads from some
buffer implementations so probably better this way with the fix in
the buffer itself as you have done here.   We should also update the
docs in buffer.h to make it clear it is in bytes rather than 'elements'
as it currently says.   If you would like to add it to this patch that
would be great.  I guess I've just been 'lucky' whilst testing this
buffer...
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/kfifo_buf.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/staging/iio/kfifo_buf.c b/drivers/staging/iio/kfifo_buf.c
> index a64ebbf..930029b 100644
> --- a/drivers/staging/iio/kfifo_buf.c
> +++ b/drivers/staging/iio/kfifo_buf.c
> @@ -161,7 +161,11 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
>  	int ret, copied;
>  	struct iio_kfifo *kf = iio_to_kfifo(r);
>  
> -	ret = kfifo_to_user(&kf->kf, buf, r->bytes_per_datum*n, &copied);
> +	if (n < r->bytes_per_datum)
> +		return -EINVAL;
> +
> +	n = rounddown(n, r->bytes_per_datum);
> +	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
>  
>  	return copied;
>  }