[PATCH 1/2] iio:buffer: make length types match kfifo types

[PATCH 1/2] iio:buffer: make length types match kfifo types

[Martin Kelly]

Currently, we use int for buffer length and bytes_per_datum. However,
kfifo uses unsigned int for length and size_t for element size. We need
to make sure these matches or we will have bugs related to overflow (in
the range between INT_MAX and UINT_MAX for length, for example).

In addition, set_bytes_per_datum uses size_t while bytes_per_datum is an
int, which would cause bugs for large values of bytes_per_datum.

Change buffer length to use unsigned int and bytes_per_datum to use
size_t.

Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
 drivers/iio/buffer/industrialio-buffer-dma.c | 2 +-
 drivers/iio/buffer/kfifo_buf.c               | 4 ++--
 include/linux/iio/buffer_impl.h              | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
index 05e0c353e089..b32bf57910ca 100644
--- a/drivers/iio/buffer/industrialio-buffer-dma.c
+++ b/drivers/iio/buffer/industrialio-buffer-dma.c
@@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_set_bytes_per_datum);
  * Should be used as the set_length callback for iio_buffer_access_ops
  * struct for DMA buffers.
  */
-int iio_dma_buffer_set_length(struct iio_buffer *buffer, int length)
+int iio_dma_buffer_set_length(struct iio_buffer *buffer, unsigned int length)
 {
 	/* Avoid an invalid state */
 	if (length < 2)
diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index 047fe757ab97..ac622edf2486 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -22,7 +22,7 @@ struct iio_kfifo {
 #define iio_to_kfifo(r) container_of(r, struct iio_kfifo, buffer)
 
 static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
-				int bytes_per_datum, int length)
+			size_t bytes_per_datum, unsigned int length)
 {
 	if ((length == 0) || (bytes_per_datum == 0))
 		return -EINVAL;
@@ -67,7 +67,7 @@ static int iio_set_bytes_per_datum_kfifo(struct iio_buffer *r, size_t bpd)
 	return 0;
 }
 
-static int iio_set_length_kfifo(struct iio_buffer *r, int length)
+static int iio_set_length_kfifo(struct iio_buffer *r, unsigned int length)
 {
 	/* Avoid an invalid state */
 	if (length < 2)
diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
index b9e22b7e2f28..d1171db23742 100644
--- a/include/linux/iio/buffer_impl.h
+++ b/include/linux/iio/buffer_impl.h
@@ -53,7 +53,7 @@ struct iio_buffer_access_funcs {
 	int (*request_update)(struct iio_buffer *buffer);
 
 	int (*set_bytes_per_datum)(struct iio_buffer *buffer, size_t bpd);
-	int (*set_length)(struct iio_buffer *buffer, int length);
+	int (*set_length)(struct iio_buffer *buffer, unsigned int length);
 
 	int (*enable)(struct iio_buffer *buffer, struct iio_dev *indio_dev);
 	int (*disable)(struct iio_buffer *buffer, struct iio_dev *indio_dev);
@@ -72,10 +72,10 @@ struct iio_buffer_access_funcs {
  */
 struct iio_buffer {
 	/** @length: Number of datums in buffer. */
-	int length;
+	unsigned int length;
 
 	/**  @bytes_per_datum: Size of individual datum including timestamp. */
-	int bytes_per_datum;
+	size_t bytes_per_datum;
 
 	/**
 	 * @access: Buffer access functions associated with the
-- 
2.11.0

[PATCH 2/2] iio:kfifo_buf: check for uint overflow

[Martin Kelly]

Currently, the following causes a kernel OOPS in memcpy:

echo 1073741825 > buffer/length
echo 1 > buffer/enable

Note that using 1073741824 instead of 1073741825 causes "write error:
Cannot allocate memory" but no OOPS.

This is because 1073741824 == 2^30 and 1073741825 == 2^30+1. Since kfifo
rounds up to the nearest power of 2, it will actually call kmalloc with
roundup_pow_of_two(length) * bytes_per_datum.

Using length == 1073741825 and bytes_per_datum == 2, we get:

kmalloc(roundup_pow_of_two(1073741825) * 2
or kmalloc(2147483648 * 2)
or kmalloc(4294967296)
or kmalloc(UINT_MAX + 1)

so this overflows to 0, causing kmalloc to return ZERO_SIZE_PTR and
subsequent memcpy to fail once the device is enabled.

Fix this by checking for overflow prior to allocating a kfifo. With this
check added, the above code returns -EINVAL when enabling the buffer,
rather than causing an OOPS.

Signed-off-by: Martin Kelly <mkelly@xevo.com>
---
 drivers/iio/buffer/kfifo_buf.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
index ac622edf2486..70c302a93d7f 100644
--- a/drivers/iio/buffer/kfifo_buf.c
+++ b/drivers/iio/buffer/kfifo_buf.c
@@ -27,6 +27,13 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
 	if ((length == 0) || (bytes_per_datum == 0))
 		return -EINVAL;
 
+	/*
+	 * Make sure we don't overflow an unsigned int after kfifo rounds up to
+	 * the next power of 2.
+	 */
+	if (roundup_pow_of_two(length) > UINT_MAX / bytes_per_datum)
+		return -EINVAL;
+
 	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
 			     bytes_per_datum, GFP_KERNEL);
 }
-- 
2.11.0

Re: [PATCH 1/2] iio:buffer: make length types match kfifo types

[Jonathan Cameron]

On Mon, 26 Mar 2018 14:27:51 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> Currently, we use int for buffer length and bytes_per_datum. However,
> kfifo uses unsigned int for length and size_t for element size. We need
> to make sure these matches or we will have bugs related to overflow (in
> the range between INT_MAX and UINT_MAX for length, for example).
> 
> In addition, set_bytes_per_datum uses size_t while bytes_per_datum is an
> int, which would cause bugs for large values of bytes_per_datum.
> 
> Change buffer length to use unsigned int and bytes_per_datum to use
> size_t.
> 
> Signed-off-by: Martin Kelly <mkelly@xevo.com>
Good bit of cleanup.

I'm fine to take these but I'm not going to rush them in as fixes
for the obvious reason that it's pretty insane to have a buffer
of anywhere near the sizes where this is going to go wrong.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with them.

Thanks,

Jonathan

> ---
>  drivers/iio/buffer/industrialio-buffer-dma.c | 2 +-
>  drivers/iio/buffer/kfifo_buf.c               | 4 ++--
>  include/linux/iio/buffer_impl.h              | 6 +++---
>  3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
> index 05e0c353e089..b32bf57910ca 100644
> --- a/drivers/iio/buffer/industrialio-buffer-dma.c
> +++ b/drivers/iio/buffer/industrialio-buffer-dma.c
> @@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_set_bytes_per_datum);
>   * Should be used as the set_length callback for iio_buffer_access_ops
>   * struct for DMA buffers.
>   */
> -int iio_dma_buffer_set_length(struct iio_buffer *buffer, int length)
> +int iio_dma_buffer_set_length(struct iio_buffer *buffer, unsigned int length)
>  {
>  	/* Avoid an invalid state */
>  	if (length < 2)
> diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> index 047fe757ab97..ac622edf2486 100644
> --- a/drivers/iio/buffer/kfifo_buf.c
> +++ b/drivers/iio/buffer/kfifo_buf.c
> @@ -22,7 +22,7 @@ struct iio_kfifo {
>  #define iio_to_kfifo(r) container_of(r, struct iio_kfifo, buffer)
>  
>  static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
> -				int bytes_per_datum, int length)
> +			size_t bytes_per_datum, unsigned int length)
>  {
>  	if ((length == 0) || (bytes_per_datum == 0))
>  		return -EINVAL;
> @@ -67,7 +67,7 @@ static int iio_set_bytes_per_datum_kfifo(struct iio_buffer *r, size_t bpd)
>  	return 0;
>  }
>  
> -static int iio_set_length_kfifo(struct iio_buffer *r, int length)
> +static int iio_set_length_kfifo(struct iio_buffer *r, unsigned int length)
>  {
>  	/* Avoid an invalid state */
>  	if (length < 2)
> diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> index b9e22b7e2f28..d1171db23742 100644
> --- a/include/linux/iio/buffer_impl.h
> +++ b/include/linux/iio/buffer_impl.h
> @@ -53,7 +53,7 @@ struct iio_buffer_access_funcs {
>  	int (*request_update)(struct iio_buffer *buffer);
>  
>  	int (*set_bytes_per_datum)(struct iio_buffer *buffer, size_t bpd);
> -	int (*set_length)(struct iio_buffer *buffer, int length);
> +	int (*set_length)(struct iio_buffer *buffer, unsigned int length);
>  
>  	int (*enable)(struct iio_buffer *buffer, struct iio_dev *indio_dev);
>  	int (*disable)(struct iio_buffer *buffer, struct iio_dev *indio_dev);
> @@ -72,10 +72,10 @@ struct iio_buffer_access_funcs {
>   */
>  struct iio_buffer {
>  	/** @length: Number of datums in buffer. */
> -	int length;
> +	unsigned int length;
>  
>  	/**  @bytes_per_datum: Size of individual datum including timestamp. */
> -	int bytes_per_datum;
> +	size_t bytes_per_datum;
>  
>  	/**
>  	 * @access: Buffer access functions associated with the

Re: [PATCH 1/2] iio:buffer: make length types match kfifo types

[Jonathan Cameron]

On Fri, 30 Mar 2018 11:10:42 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 26 Mar 2018 14:27:51 -0700
> Martin Kelly <mkelly@xevo.com> wrote:
> 
> > Currently, we use int for buffer length and bytes_per_datum. However,
> > kfifo uses unsigned int for length and size_t for element size. We need
> > to make sure these matches or we will have bugs related to overflow (in
> > the range between INT_MAX and UINT_MAX for length, for example).
> > 
> > In addition, set_bytes_per_datum uses size_t while bytes_per_datum is an
> > int, which would cause bugs for large values of bytes_per_datum.
> > 
> > Change buffer length to use unsigned int and bytes_per_datum to use
> > size_t.
> > 
> > Signed-off-by: Martin Kelly <mkelly@xevo.com>  
> Good bit of cleanup.
> 
> I'm fine to take these but I'm not going to rush them in as fixes
> for the obvious reason that it's pretty insane to have a buffer
> of anywhere near the sizes where this is going to go wrong.
> 
> Applied to the togreg branch of iio.git and pushed out as testing
> for the autobuilders to play with them.
Actually I changed my mind on this after reading the second patch.
Both added to the fixes-togreg branch of iio.git and marked for
stable.  Still bonkers sized buffers, but the results are nasty
enough to make it sensible to deal with it.

So still not that urgent (which is handy given where we are in the
cycle) but nice to clean up.

Jonathan
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> >  drivers/iio/buffer/industrialio-buffer-dma.c | 2 +-
> >  drivers/iio/buffer/kfifo_buf.c               | 4 ++--
> >  include/linux/iio/buffer_impl.h              | 6 +++---
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/iio/buffer/industrialio-buffer-dma.c b/drivers/iio/buffer/industrialio-buffer-dma.c
> > index 05e0c353e089..b32bf57910ca 100644
> > --- a/drivers/iio/buffer/industrialio-buffer-dma.c
> > +++ b/drivers/iio/buffer/industrialio-buffer-dma.c
> > @@ -587,7 +587,7 @@ EXPORT_SYMBOL_GPL(iio_dma_buffer_set_bytes_per_datum);
> >   * Should be used as the set_length callback for iio_buffer_access_ops
> >   * struct for DMA buffers.
> >   */
> > -int iio_dma_buffer_set_length(struct iio_buffer *buffer, int length)
> > +int iio_dma_buffer_set_length(struct iio_buffer *buffer, unsigned int length)
> >  {
> >  	/* Avoid an invalid state */
> >  	if (length < 2)
> > diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> > index 047fe757ab97..ac622edf2486 100644
> > --- a/drivers/iio/buffer/kfifo_buf.c
> > +++ b/drivers/iio/buffer/kfifo_buf.c
> > @@ -22,7 +22,7 @@ struct iio_kfifo {
> >  #define iio_to_kfifo(r) container_of(r, struct iio_kfifo, buffer)
> >  
> >  static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
> > -				int bytes_per_datum, int length)
> > +			size_t bytes_per_datum, unsigned int length)
> >  {
> >  	if ((length == 0) || (bytes_per_datum == 0))
> >  		return -EINVAL;
> > @@ -67,7 +67,7 @@ static int iio_set_bytes_per_datum_kfifo(struct iio_buffer *r, size_t bpd)
> >  	return 0;
> >  }
> >  
> > -static int iio_set_length_kfifo(struct iio_buffer *r, int length)
> > +static int iio_set_length_kfifo(struct iio_buffer *r, unsigned int length)
> >  {
> >  	/* Avoid an invalid state */
> >  	if (length < 2)
> > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h
> > index b9e22b7e2f28..d1171db23742 100644
> > --- a/include/linux/iio/buffer_impl.h
> > +++ b/include/linux/iio/buffer_impl.h
> > @@ -53,7 +53,7 @@ struct iio_buffer_access_funcs {
> >  	int (*request_update)(struct iio_buffer *buffer);
> >  
> >  	int (*set_bytes_per_datum)(struct iio_buffer *buffer, size_t bpd);
> > -	int (*set_length)(struct iio_buffer *buffer, int length);
> > +	int (*set_length)(struct iio_buffer *buffer, unsigned int length);
> >  
> >  	int (*enable)(struct iio_buffer *buffer, struct iio_dev *indio_dev);
> >  	int (*disable)(struct iio_buffer *buffer, struct iio_dev *indio_dev);
> > @@ -72,10 +72,10 @@ struct iio_buffer_access_funcs {
> >   */
> >  struct iio_buffer {
> >  	/** @length: Number of datums in buffer. */
> > -	int length;
> > +	unsigned int length;
> >  
> >  	/**  @bytes_per_datum: Size of individual datum including timestamp. */
> > -	int bytes_per_datum;
> > +	size_t bytes_per_datum;
> >  
> >  	/**
> >  	 * @access: Buffer access functions associated with the  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] iio:kfifo_buf: check for uint overflow

[Jonathan Cameron]

On Mon, 26 Mar 2018 14:27:52 -0700
Martin Kelly <mkelly@xevo.com> wrote:

> Currently, the following causes a kernel OOPS in memcpy:
> 
> echo 1073741825 > buffer/length
> echo 1 > buffer/enable
> 
> Note that using 1073741824 instead of 1073741825 causes "write error:
> Cannot allocate memory" but no OOPS.
> 
> This is because 1073741824 == 2^30 and 1073741825 == 2^30+1. Since kfifo
> rounds up to the nearest power of 2, it will actually call kmalloc with
> roundup_pow_of_two(length) * bytes_per_datum.
> 
> Using length == 1073741825 and bytes_per_datum == 2, we get:
> 
> kmalloc(roundup_pow_of_two(1073741825) * 2
> or kmalloc(2147483648 * 2)
> or kmalloc(4294967296)
> or kmalloc(UINT_MAX + 1)
> 
> so this overflows to 0, causing kmalloc to return ZERO_SIZE_PTR and
> subsequent memcpy to fail once the device is enabled.
> 
> Fix this by checking for overflow prior to allocating a kfifo. With this
> check added, the above code returns -EINVAL when enabling the buffer,
> rather than causing an OOPS.
> 
> Signed-off-by: Martin Kelly <mkelly@xevo.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

I thought about this for a few mins.  A 4 gig allocation on a 32bit
machine is going to fail anyway for obvious reasons, but good
to protect against overflow if someone were to write this value.

Particularly good description btw!

Thanks,

Jonathan


> ---
>  drivers/iio/buffer/kfifo_buf.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
> index ac622edf2486..70c302a93d7f 100644
> --- a/drivers/iio/buffer/kfifo_buf.c
> +++ b/drivers/iio/buffer/kfifo_buf.c
> @@ -27,6 +27,13 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
>  	if ((length == 0) || (bytes_per_datum == 0))
>  		return -EINVAL;
>  
> +	/*
> +	 * Make sure we don't overflow an unsigned int after kfifo rounds up to
> +	 * the next power of 2.
> +	 */
> +	if (roundup_pow_of_two(length) > UINT_MAX / bytes_per_datum)
> +		return -EINVAL;
> +
>  	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
>  			     bytes_per_datum, GFP_KERNEL);
>  }

Re: [PATCH 2/2] iio:kfifo_buf: check for uint overflow

[Martin Kelly]

On 03/30/2018 03:20 AM, Jonathan Cameron wrote:
> On Mon, 26 Mar 2018 14:27:52 -0700
> Martin Kelly <mkelly@xevo.com> wrote:
> 
>> Currently, the following causes a kernel OOPS in memcpy:
>>
>> echo 1073741825 > buffer/length
>> echo 1 > buffer/enable
>>
>> Note that using 1073741824 instead of 1073741825 causes "write error:
>> Cannot allocate memory" but no OOPS.
>>
>> This is because 1073741824 == 2^30 and 1073741825 == 2^30+1. Since kfifo
>> rounds up to the nearest power of 2, it will actually call kmalloc with
>> roundup_pow_of_two(length) * bytes_per_datum.
>>
>> Using length == 1073741825 and bytes_per_datum == 2, we get:
>>
>> kmalloc(roundup_pow_of_two(1073741825) * 2
>> or kmalloc(2147483648 * 2)
>> or kmalloc(4294967296)
>> or kmalloc(UINT_MAX + 1)
>>
>> so this overflows to 0, causing kmalloc to return ZERO_SIZE_PTR and
>> subsequent memcpy to fail once the device is enabled.
>>
>> Fix this by checking for overflow prior to allocating a kfifo. With this
>> check added, the above code returns -EINVAL when enabling the buffer,
>> rather than causing an OOPS.
>>
>> Signed-off-by: Martin Kelly <mkelly@xevo.com>
> Applied to the fixes-togreg branch of iio.git and marked for stable.
> 
> I thought about this for a few mins.  A 4 gig allocation on a 32bit
> machine is going to fail anyway for obvious reasons, but good
> to protect against overflow if someone were to write this value.
> 

Yep, I found this by accident when my userspace had a bug, and then I 
just got curious about what was happening. Such clean, isolated bugs 
like this are rare and fun to find :).

> Particularly good description btw!
> 
> Thanks,
> 
> Jonathan
> 
> 
>> ---
>>   drivers/iio/buffer/kfifo_buf.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/iio/buffer/kfifo_buf.c b/drivers/iio/buffer/kfifo_buf.c
>> index ac622edf2486..70c302a93d7f 100644
>> --- a/drivers/iio/buffer/kfifo_buf.c
>> +++ b/drivers/iio/buffer/kfifo_buf.c
>> @@ -27,6 +27,13 @@ static inline int __iio_allocate_kfifo(struct iio_kfifo *buf,
>>   	if ((length == 0) || (bytes_per_datum == 0))
>>   		return -EINVAL;
>>   
>> +	/*
>> +	 * Make sure we don't overflow an unsigned int after kfifo rounds up to
>> +	 * the next power of 2.
>> +	 */
>> +	if (roundup_pow_of_two(length) > UINT_MAX / bytes_per_datum)
>> +		return -EINVAL;
>> +
>>   	return __kfifo_alloc((struct __kfifo *)&buf->kf, length,
>>   			     bytes_per_datum, GFP_KERNEL);
>>   }
>