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

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

[Lars-Peter Clausen]

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.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 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;
 }
-- 
1.7.7.3

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

[Jonathan Cameron]

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;
>  }

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

[Jonathan Cameron]

On 12/08/2011 09:14 AM, Jonathan Cameron wrote:
> 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>
Doh, forgot I'm not using that email address for acks anymore.
Acked-by: Jonathan Cameron <jic23@kernel.org>

pesky finger memory...
>> ---
>>  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;
>>  }
> 

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

[Lars-Peter Clausen]

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.

Also update the documentation of read_first_n to reflect the fact that 'n' is
supposed to be in bytes and not in samples.

Acked-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/buffer.h    |    2 +-
 drivers/staging/iio/kfifo_buf.c |    6 +++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/buffer.h b/drivers/staging/iio/buffer.h
index c697099..44593b2 100644
--- a/drivers/staging/iio/buffer.h
+++ b/drivers/staging/iio/buffer.h
@@ -21,7 +21,7 @@ struct iio_buffer;
  * @mark_in_use:	reference counting, typically to prevent module removal
  * @unmark_in_use:	reduce reference count when no longer using buffer
  * @store_to:		actually store stuff to the buffer
- * @read_first_n:	try to get a specified number of elements (must exist)
+ * @read_first_n:	try to get a specified number of bytes (must exist)
  * @mark_param_change:	notify buffer that some relevant parameter has changed
  *			Often this means the underlying storage may need to
  *			change.
diff --git a/drivers/staging/iio/kfifo_buf.c b/drivers/staging/iio/kfifo_buf.c
index d8867ab..b69cca5 100644
--- a/drivers/staging/iio/kfifo_buf.c
+++ b/drivers/staging/iio/kfifo_buf.c
@@ -160,7 +160,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;
 }
-- 
1.7.7.3