| Message ID | 20180326212752.7321-2-mkelly@xevo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > } -- 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
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); >> } > -- 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
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); }
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(+)