[PATCH 1/3] staging:iio: Fix sw_ring memory corruption

[PATCH 1/3] staging:iio: Fix sw_ring memory corruption

[Lars-Peter Clausen]

The sw_ring does not properly handle the case where the write pointer already
has wrapped around, the read pointer has not and the remaining buffer space at
the end is enough to fill the read buffer:

  +-----------------------------------+
  |     |              |##data##|     |
  +-----------------------------------+
     write_p        read_p

In this case the current code will copy all available data to the buffer and
as a result will write beyond the bounds of the buffer and cause a memory
corruption.

To address this issue this patch adds code to calculate the available buffer
space and makes sure that the number of bytes to copy does not exceed this
number. This allows the code which copies the data around to be simplified as
it only has to consider two cases: Read wraps around and read does not wrap
around.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/staging/iio/ring_sw.c |   58 ++++++++++++++++------------------------
 1 files changed, 23 insertions(+), 35 deletions(-)

diff --git a/drivers/staging/iio/ring_sw.c b/drivers/staging/iio/ring_sw.c
index a541a73..4c15cc8 100644
--- a/drivers/staging/iio/ring_sw.c
+++ b/drivers/staging/iio/ring_sw.c
@@ -170,6 +170,7 @@ static int iio_read_first_n_sw_rb(struct iio_buffer *r,
 	u8 *initial_read_p, *initial_write_p, *current_read_p, *end_read_p;
 	u8 *data;
 	int ret, max_copied, bytes_to_rip, dead_offset;
+	size_t data_available, buffer_size;
 
 	/* A userspace program has probably made an error if it tries to
 	 *  read something that is not a whole number of bpds.
@@ -182,9 +183,11 @@ static int iio_read_first_n_sw_rb(struct iio_buffer *r,
 		       n, ring->buf.bytes_per_datum);
 		goto error_ret;
 	}
+
+	buffer_size = ring->buf.bytes_per_datum*ring->buf.length;
+
 	/* Limit size to whole of ring buffer */
-	bytes_to_rip = min((size_t)(ring->buf.bytes_per_datum*ring->buf.length),
-			   n);
+	bytes_to_rip = min_t(size_t, buffer_size, n);
 
 	data = kmalloc(bytes_to_rip, GFP_KERNEL);
 	if (data == NULL) {
@@ -213,38 +216,24 @@ static int iio_read_first_n_sw_rb(struct iio_buffer *r,
 		goto error_free_data_cpy;
 	}
 
-	if (initial_write_p >= initial_read_p + bytes_to_rip) {
-		/* write_p is greater than necessary, all is easy */
-		max_copied = bytes_to_rip;
-		memcpy(data, initial_read_p, max_copied);
-		end_read_p = initial_read_p + max_copied;
-	} else if (initial_write_p > initial_read_p) {
-		/*not enough data to cpy */
-		max_copied = initial_write_p - initial_read_p;
+	if (initial_write_p >= initial_read_p)
+		data_available = initial_write_p - initial_read_p;
+	else
+		data_available = buffer_size - (initial_read_p - initial_write_p);
+
+	if (data_available < bytes_to_rip)
+		bytes_to_rip = data_available;
+
+	if (initial_read_p + bytes_to_rip >= ring->data + buffer_size) {
+		max_copied = ring->data + buffer_size - initial_read_p;
 		memcpy(data, initial_read_p, max_copied);
-		end_read_p = initial_write_p;
+		memcpy(data + max_copied, ring->data, bytes_to_rip - max_copied);
+		end_read_p = ring->data + bytes_to_rip - max_copied;
 	} else {
-		/* going through 'end' of ring buffer */
-		max_copied = ring->data
-			+ ring->buf.length*ring->buf.bytes_per_datum - initial_read_p;
-		memcpy(data, initial_read_p, max_copied);
-		/* possible we are done if we align precisely with end */
-		if (max_copied == bytes_to_rip)
-			end_read_p = ring->data;
-		else if (initial_write_p
-			 > ring->data + bytes_to_rip - max_copied) {
-			/* enough data to finish */
-			memcpy(data + max_copied, ring->data,
-			       bytes_to_rip - max_copied);
-			max_copied = bytes_to_rip;
-			end_read_p = ring->data + (bytes_to_rip - max_copied);
-		} else {  /* not enough data */
-			memcpy(data + max_copied, ring->data,
-			       initial_write_p - ring->data);
-			max_copied += initial_write_p - ring->data;
-			end_read_p = initial_write_p;
-		}
+		memcpy(data, initial_read_p, bytes_to_rip);
+		end_read_p = initial_read_p + bytes_to_rip;
 	}
+
 	/* Now to verify which section was cleanly copied - i.e. how far
 	 * read pointer has been pushed */
 	current_read_p = ring->read_p;
@@ -252,15 +241,14 @@ static int iio_read_first_n_sw_rb(struct iio_buffer *r,
 	if (initial_read_p <= current_read_p)
 		dead_offset = current_read_p - initial_read_p;
 	else
-		dead_offset = ring->buf.length*ring->buf.bytes_per_datum
-			- (initial_read_p - current_read_p);
+		dead_offset = buffer_size - (initial_read_p - current_read_p);
 
 	/* possible issue if the initial write has been lapped or indeed
 	 * the point we were reading to has been passed */
 	/* No valid data read.
 	 * In this case the read pointer is already correct having been
 	 * pushed further than we would look. */
-	if (max_copied - dead_offset < 0) {
+	if (bytes_to_rip - dead_offset < 0) {
 		ret = 0;
 		goto error_free_data_cpy;
 	}
@@ -276,7 +264,7 @@ static int iio_read_first_n_sw_rb(struct iio_buffer *r,
 	while (ring->read_p != end_read_p)
 		ring->read_p = end_read_p;
 
-	ret = max_copied - dead_offset;
+	ret = bytes_to_rip - dead_offset;
 
 	if (copy_to_user(buf, data + dead_offset, ret))  {
 		ret =  -EFAULT;
-- 
1.7.7.3

[PATCH 2/3] 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

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

[Lars-Peter Clausen]

On 12/08/2011 09:17 PM, Greg KH wrote:
> On Thu, Dec 08, 2011 at 06:35:52PM +0100, 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.
>>
>> 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 +-
> 
> This patch no longer applies to my tree, care to refresh it and resend
> it?

Hi Greg,

I just rebased the patch on top of the current staging-next and it is
identically to this one. Could you try to re-apply it, if you still have it?
Otherwise I'll send it out again.

Thanks,
- Lars

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

[Greg KH]

On Fri, Dec 09, 2011 at 10:58:19AM +0100, Lars-Peter Clausen wrote:
> On 12/08/2011 09:17 PM, Greg KH wrote:
> > On Thu, Dec 08, 2011 at 06:35:52PM +0100, 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.
> >>
> >> 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 +-
> > 
> > This patch no longer applies to my tree, care to refresh it and resend
> > it?
> 
> Hi Greg,
> 
> I just rebased the patch on top of the current staging-next and it is
> identically to this one. Could you try to re-apply it, if you still have it?
> Otherwise I'll send it out again.

Can you resend it?  I don't know what I did wrong here...

greg k-h

[PATCH 3/3] staging:iio: Fix scan mask update

[Lars-Peter Clausen]

When updating the scan mask we have to check the actual scan mask for if the
channel is already enabled, not the matching scan mask from the available
scan masks. The bit will already be set there and as a result the actual
scan mask will not get updated and the channel stays disabled.

Also fix the return value of iio_scan_el_store which would return 1 instead of
the number of bytes written if the channel was already active in the scan mask.

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

diff --git a/drivers/staging/iio/industrialio-buffer.c b/drivers/staging/iio/industrialio-buffer.c
index 552e33f..a03a574 100644
--- a/drivers/staging/iio/industrialio-buffer.c
+++ b/drivers/staging/iio/industrialio-buffer.c
@@ -173,7 +173,7 @@ static ssize_t iio_scan_el_store(struct device *dev,
 error_ret:
 	mutex_unlock(&indio_dev->mlock);
 
-	return ret ? ret : len;
+	return ret < 0 ? ret : len;
 
 }
 
@@ -624,16 +624,8 @@ int iio_scan_mask_query(struct iio_dev *indio_dev,
 
 	if (!buffer->scan_mask)
 		return 0;
-	if (indio_dev->available_scan_masks)
-		mask = iio_scan_mask_match(indio_dev->available_scan_masks,
-					   indio_dev->masklength,
-					   buffer->scan_mask);
-	else
-		mask = buffer->scan_mask;
-	if (!mask)
-		return 0;
 
-	return test_bit(bit, mask);
+	return test_bit(bit, buffer->scan_mask);
 };
 EXPORT_SYMBOL_GPL(iio_scan_mask_query);
 
-- 
1.7.7.3

Re: [PATCH 3/3] staging:iio: Fix scan mask update

[Greg KH]

On Thu, Dec 08, 2011 at 06:35:53PM +0100, Lars-Peter Clausen wrote:
> When updating the scan mask we have to check the actual scan mask for=
 if the
> channel is already enabled, not the matching scan mask from the avail=
able
> scan masks. The bit will already be set there and as a result the act=
ual
> scan mask will not get updated and the channel stays disabled.
>=20
> Also fix the return value of iio_scan_el_store which would return 1 i=
nstead of
> the number of bytes written if the channel was already active in the =
scan mask.

You just added a complier warning with this patch:
  CC [M]  drivers/staging/iio/industrialio-buffer.o
drivers/staging/iio/industrialio-buffer.c: In function =E2=80=98iio_sca=
n_mask_query=E2=80=99:
drivers/staging/iio/industrialio-buffer.c:621:8: warning: unused variab=
le =E2=80=98mask=E2=80=99 [-Wunused-variable]

So I didn't apply it.

Care to try again and be more careful in the future?

thanks,

greg k-h

Re: [PATCH 3/3] staging:iio: Fix scan mask update

[Lars-Peter Clausen]

On 12/08/2011 09:18 PM, Greg KH wrote:
> On Thu, Dec 08, 2011 at 06:35:53PM +0100, Lars-Peter Clausen wrote:
>> When updating the scan mask we have to check the actual scan mask fo=
r if the
>> channel is already enabled, not the matching scan mask from the avai=
lable
>> scan masks. The bit will already be set there and as a result the ac=
tual
>> scan mask will not get updated and the channel stays disabled.
>>
>> Also fix the return value of iio_scan_el_store which would return 1 =
instead of
>> the number of bytes written if the channel was already active in the=
 scan mask.
>=20
> You just added a complier warning with this patch:
>   CC [M]  drivers/staging/iio/industrialio-buffer.o
> drivers/staging/iio/industrialio-buffer.c: In function =E2=80=98iio_s=
can_mask_query=E2=80=99:
> drivers/staging/iio/industrialio-buffer.c:621:8: warning: unused vari=
able =E2=80=98mask=E2=80=99 [-Wunused-variable]
>=20
> So I didn't apply it.

Actually you did. At least it is in staging-next. Should I resend a upd=
ated
patch or just a patch removing the now unused mask variable.

Thanks,
- Lars

Re: [PATCH 3/3] staging:iio: Fix scan mask update

[Greg KH]

On Fri, Dec 09, 2011 at 10:49:45AM +0100, Lars-Peter Clausen wrote:
> On 12/08/2011 09:18 PM, Greg KH wrote:
> > On Thu, Dec 08, 2011 at 06:35:53PM +0100, Lars-Peter Clausen wrote:
> >> When updating the scan mask we have to check the actual scan mask =
for if the
> >> channel is already enabled, not the matching scan mask from the av=
ailable
> >> scan masks. The bit will already be set there and as a result the =
actual
> >> scan mask will not get updated and the channel stays disabled.
> >>
> >> Also fix the return value of iio_scan_el_store which would return =
1 instead of
> >> the number of bytes written if the channel was already active in t=
he scan mask.
> >=20
> > You just added a complier warning with this patch:
> >   CC [M]  drivers/staging/iio/industrialio-buffer.o
> > drivers/staging/iio/industrialio-buffer.c: In function =E2=80=98iio=
_scan_mask_query=E2=80=99:
> > drivers/staging/iio/industrialio-buffer.c:621:8: warning: unused va=
riable =E2=80=98mask=E2=80=99 [-Wunused-variable]
> >=20
> > So I didn't apply it.
>=20
> Actually you did. At least it is in staging-next. Should I resend a u=
pdated
> patch or just a patch removing the now unused mask variable.

Ugh, you are right, I was messing with my scripts to apply patches at
the time, and forgot to delete this branch.

So, please send a follow-on patch to fix the warning.

greg k-h

Re: [PATCH 3/3] staging:iio: Fix scan mask update

[Jonathan Cameron]

On 12/09/2011 03:11 PM, Greg KH wrote:
> On Fri, Dec 09, 2011 at 10:49:45AM +0100, Lars-Peter Clausen wrote:
>> On 12/08/2011 09:18 PM, Greg KH wrote:
>>> On Thu, Dec 08, 2011 at 06:35:53PM +0100, Lars-Peter Clausen wrote:
>>>> When updating the scan mask we have to check the actual scan mask =
for if the
>>>> channel is already enabled, not the matching scan mask from the av=
ailable
>>>> scan masks. The bit will already be set there and as a result the =
actual
>>>> scan mask will not get updated and the channel stays disabled.
>>>>
>>>> Also fix the return value of iio_scan_el_store which would return =
1 instead of
>>>> the number of bytes written if the channel was already active in t=
he scan mask.
>>>
>>> You just added a complier warning with this patch:
>>>   CC [M]  drivers/staging/iio/industrialio-buffer.o
>>> drivers/staging/iio/industrialio-buffer.c: In function =E2=80=98iio=
_scan_mask_query=E2=80=99:
>>> drivers/staging/iio/industrialio-buffer.c:621:8: warning: unused va=
riable =E2=80=98mask=E2=80=99 [-Wunused-variable]
>>>
>>> So I didn't apply it.
>>
>> Actually you did. At least it is in staging-next. Should I resend a =
updated
>> patch or just a patch removing the now unused mask variable.
>=20
> Ugh, you are right, I was messing with my scripts to apply patches at
> the time, and forgot to delete this branch.
>=20
> So, please send a follow-on patch to fix the warning.
>=20
Lars-Peter, have you sent such a patch to Greg?  I'm still seeing this
warning hence the bump! Just wondering if this got lost in amongst
everything else that is going on.

Re: [PATCH 3/3] staging:iio: Fix scan mask update

[Lars-Peter Clausen]

On 12/18/2011 07:02 PM, Jonathan Cameron wrote:
> On 12/09/2011 03:11 PM, Greg KH wrote:
>> On Fri, Dec 09, 2011 at 10:49:45AM +0100, Lars-Peter Clausen wrote:
>>> On 12/08/2011 09:18 PM, Greg KH wrote:
>>>> On Thu, Dec 08, 2011 at 06:35:53PM +0100, Lars-Peter Clausen wrote=
:
>>>>> When updating the scan mask we have to check the actual scan mask=
 for if the
>>>>> channel is already enabled, not the matching scan mask from the a=
vailable
>>>>> scan masks. The bit will already be set there and as a result the=
 actual
>>>>> scan mask will not get updated and the channel stays disabled.
>>>>>
>>>>> Also fix the return value of iio_scan_el_store which would return=
 1 instead of
>>>>> the number of bytes written if the channel was already active in =
the scan mask.
>>>>
>>>> You just added a complier warning with this patch:
>>>>   CC [M]  drivers/staging/iio/industrialio-buffer.o
>>>> drivers/staging/iio/industrialio-buffer.c: In function =E2=80=98ii=
o_scan_mask_query=E2=80=99:
>>>> drivers/staging/iio/industrialio-buffer.c:621:8: warning: unused v=
ariable =E2=80=98mask=E2=80=99 [-Wunused-variable]
>>>>
>>>> So I didn't apply it.
>>>
>>> Actually you did. At least it is in staging-next. Should I resend a=
 updated
>>> patch or just a patch removing the now unused mask variable.
>>
>> Ugh, you are right, I was messing with my scripts to apply patches a=
t
>> the time, and forgot to delete this branch.
>>
>> So, please send a follow-on patch to fix the warning.
>>
> Lars-Peter, have you sent such a patch to Greg?  I'm still seeing thi=
s
> warning hence the bump! Just wondering if this got lost in amongst
> everything else that is going on.

Haven't send it out yet. Will do tomorrow.