gpio: prevent potential speculation leaks in gpio_device_get_desc()

Users can call the gpio_ioctl() interface to get information about gpio
chip lines.
Lines on the chip are identified by an offset in the range
of [0,chip.lines).
Offset is copied from user and then used as an array index to get
the gpio descriptor without sanitization.

This change ensures that the offset is sanitized by
using "array_index_nospec" to mitigate any possibility of speculative
information leaks.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
Signed-off-by: Hagar Hemdan <hagarhem@amazon.com>
---
Only compile tested, no access to HW.
---
 drivers/gpio/gpiolib-cdev.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

On Tue, May 14, 2024 at 12:26:01PM +0000, Hagar Hemdan wrote:
> Users can call the gpio_ioctl() interface to get information about gpio
> chip lines.

Indeed they can, assuming they have access to the gpiochip device. So what?

> Lines on the chip are identified by an offset in the range
> of [0,chip.lines).
> Offset is copied from user and then used as an array index to get
> the gpio descriptor without sanitization.

Yup, and it returns an -EINVAL, via gpio_device_get_desc(), if it is out
of range.

>
> This change ensures that the offset is sanitized by
> using "array_index_nospec" to mitigate any possibility of speculative
> information leaks.
>

Speculative leaks of what?  The size of the array?
That is explicitly public knowledge - if they call GPIO_GET_CHIPINFO_IOCTL
it will tell them.

> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
>
> Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
> Signed-off-by: Hagar Hemdan <hagarhem@amazon.com>
> ---
> Only compile tested, no access to HW.
> ---
>  drivers/gpio/gpiolib-cdev.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index 9dad67ea2597..215c03e6808f 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -20,6 +20,7 @@
>  #include <linux/kfifo.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/nospec.h>
>  #include <linux/overflow.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/poll.h>
> @@ -2170,7 +2171,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
>  	lflags = eventreq.handleflags;
>  	eflags = eventreq.eventflags;
>
> -	desc = gpio_device_get_desc(gdev, offset);
> +	desc = gpio_device_get_desc(gdev,
> +				array_index_nospec(offset, gdev->ngpio));

Moving an out of bounds index INTO bounds here is totally wrong.
That is NOT what the user asked for, and in that case they should get an
error, as they currently do, no an actual different line - which is what
this change does.

NACK.

Cheers,
Kent.

>  	if (IS_ERR(desc))
>  		return PTR_ERR(desc);
>
> @@ -2477,7 +2479,8 @@ static int lineinfo_get_v1(struct gpio_chardev_data *cdev, void __user *ip,
>  		return -EFAULT;
>
>  	/* this doubles as a range check on line_offset */
> -	desc = gpio_device_get_desc(cdev->gdev, lineinfo.line_offset);
> +	desc = gpio_device_get_desc(cdev->gdev,
> +				array_index_nospec(lineinfo.line_offset, cdev->gdev->ngpio));
>  	if (IS_ERR(desc))
>  		return PTR_ERR(desc);
>
> @@ -2514,7 +2517,8 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
>  	if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding)))
>  		return -EINVAL;
>
> -	desc = gpio_device_get_desc(cdev->gdev, lineinfo.offset);
> +	desc = gpio_device_get_desc(cdev->gdev,
> +				array_index_nospec(lineinfo.offset, cdev->gdev->ngpio));
>  	if (IS_ERR(desc))
>  		return PTR_ERR(desc);
>
> --
> 2.40.1
>

On Tue, May 14, 2024 at 08:42:21PM +0800, Kent Gibson wrote:
> On Tue, May 14, 2024 at 12:26:01PM +0000, Hagar Hemdan wrote:
> > Users can call the gpio_ioctl() interface to get information about gpio
> > chip lines.
> 
> Indeed they can, assuming they have access to the gpiochip device. So what?
> 
> > Lines on the chip are identified by an offset in the range
> > of [0,chip.lines).
> > Offset is copied from user and then used as an array index to get
> > the gpio descriptor without sanitization.
> 
> Yup, and it returns an -EINVAL, via gpio_device_get_desc(), if it is out
> of range.
> 
 In case of speculation executin, the condition (hwnum >= gdev→ngpio)
may be miss predicted as true and then the value of &gdev→descs[hwnum] is
speculatively loaded even if hwnum >= gdev→ngpio.

> >
> > This change ensures that the offset is sanitized by
> > using "array_index_nospec" to mitigate any possibility of speculative
> > information leaks.
> >
> 
> Speculative leaks of what?  The size of the array?
> That is explicitly public knowledge - if they call GPIO_GET_CHIPINFO_IOCTL
> it will tell them.
>
Speculation leaks of gdev→descs[hwnum] when hwnum >= ngpio.
As in func "lineinfo_get()", hwnum is an offset copied from user and used
as an index to get a device descriptor in gpio_device_get_desc().
Could you explain what do you mean by it is a public knowledge?

> > This bug was discovered and resolved using Coverity Static Analysis
> > Security Testing (SAST) by Synopsys, Inc.
> >
> > Fixes: aad955842d1c ("gpiolib: cdev: support GPIO_V2_GET_LINEINFO_IOCTL and GPIO_V2_GET_LINEINFO_WATCH_IOCTL")
> > Signed-off-by: Hagar Hemdan <hagarhem@amazon.com>
> > ---
> > Only compile tested, no access to HW.
> > ---
> >  drivers/gpio/gpiolib-cdev.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index 9dad67ea2597..215c03e6808f 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/kfifo.h>
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> > +#include <linux/nospec.h>
> >  #include <linux/overflow.h>
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/poll.h>
> > @@ -2170,7 +2171,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> >  	lflags = eventreq.handleflags;
> >  	eflags = eventreq.eventflags;
> >
> > -	desc = gpio_device_get_desc(gdev, offset);
> > +	desc = gpio_device_get_desc(gdev,
> > +				array_index_nospec(offset, gdev->ngpio));
> 
> Moving an out of bounds index INTO bounds here is totally wrong.
> That is NOT what the user asked for, and in that case they should get an
> error, as they currently do, no an actual different line - which is what
> this change does.
> 
> NACK.
> 
> Cheers,
> Kent.
This macro "array_index_nospec()" prevents out-of-bounds accesses
under speculation execution, ensures that bounds checks are
respected even under speculation and not moving out of bounds into bounds.

> 
> >  	if (IS_ERR(desc))
> >  		return PTR_ERR(desc);
> >
> > @@ -2477,7 +2479,8 @@ static int lineinfo_get_v1(struct gpio_chardev_data *cdev, void __user *ip,
> >  		return -EFAULT;
> >
> >  	/* this doubles as a range check on line_offset */
> > -	desc = gpio_device_get_desc(cdev->gdev, lineinfo.line_offset);
> > +	desc = gpio_device_get_desc(cdev->gdev,
> > +				array_index_nospec(lineinfo.line_offset, cdev->gdev->ngpio));
> >  	if (IS_ERR(desc))
> >  		return PTR_ERR(desc);
> >
> > @@ -2514,7 +2517,8 @@ static int lineinfo_get(struct gpio_chardev_data *cdev, void __user *ip,
> >  	if (memchr_inv(lineinfo.padding, 0, sizeof(lineinfo.padding)))
> >  		return -EINVAL;
> >
> > -	desc = gpio_device_get_desc(cdev->gdev, lineinfo.offset);
> > +	desc = gpio_device_get_desc(cdev->gdev,
> > +				array_index_nospec(lineinfo.offset, cdev->gdev->ngpio));
> >  	if (IS_ERR(desc))
> >  		return PTR_ERR(desc);
> >
> > --
> > 2.40.1
> >

On Thu, May 16, 2024 at 12:57:42PM +0000, Hagar Hemdan wrote:
> On Tue, May 14, 2024 at 08:42:21PM +0800, Kent Gibson wrote:
> > On Tue, May 14, 2024 at 12:26:01PM +0000, Hagar Hemdan wrote:
> > > Users can call the gpio_ioctl() interface to get information about gpio
> > > chip lines.
> >
> > Indeed they can, assuming they have access to the gpiochip device. So what?
> >
> > > Lines on the chip are identified by an offset in the range
> > > of [0,chip.lines).
> > > Offset is copied from user and then used as an array index to get
> > > the gpio descriptor without sanitization.
> >
> > Yup, and it returns an -EINVAL, via gpio_device_get_desc(), if it is out
> > of range.
> >
>  In case of speculation executin, the condition (hwnum >= gdev→ngpio)
> may be miss predicted as true and then the value of &gdev→descs[hwnum] is
> speculatively loaded even if hwnum >= gdev→ngpio.
>

Ok, I mis-understood the problem.  I probably still don't understand it.
The problem is that userspace may trigger a speculative read of an
address outside the array, and that is a problem, even if that address is
never returned, as userspace can trigger speculative reads of arbitrary
addresses?

And the fix is in cdev, rather than gpio_device_get_desc(), as the
offset in cdev is known to be from userspace?

> This macro "array_index_nospec()" prevents out-of-bounds accesses
> under speculation execution, ensures that bounds checks are
> respected even under speculation and not moving out of bounds into bounds.

In that case, I clearly don't understand what array_index_nospec() is doing,
as if it does NOT alter the offset being passed to gpio_device_get_desc()
then it must be performing some serious voodoo to be changing the behaviour
of gpio_device_get_desc() which performs the actual range check and indexing.

Its doco says this:

/*
 * array_index_nospec - sanitize an array index after a bounds check
 *
 * For a code sequence like:
 *
 *     if (index < size) {
 *         index = array_index_nospec(index, size);
 *         val = array[index];
 *     }
 *
 * ...if the CPU speculates past the bounds check then
 * array_index_nospec() will clamp the index within the range of [0,
 * size).
 */

Looks to me like it should be applied AFTER the bounds check.
And the code appears to apply a mask to the index:

	(typeof(_i)) (_i & _mask);					\

Now I need to test your patch to see what it actually does.

But assuming it does fix the issue, without changing the offset being
referenced, you might want to check ALL the places that cdev calls
gpio_device_get_desc() - you missed a couple.
I count five, you patched three.  What is special about the two you missed?

For both reasons, perhaps it would be more appropriate to perform the
array_index_nospec() in gpio_device_get_desc() itself?

Cheers,
Kent.

On Thu, May 16, 2024 at 10:55:40PM +0800, Kent Gibson wrote:
> On Thu, May 16, 2024 at 12:57:42PM +0000, Hagar Hemdan wrote:
> > On Tue, May 14, 2024 at 08:42:21PM +0800, Kent Gibson wrote:
>
> Now I need to test your patch to see what it actually does.
>

Tested.  Fails.  It does what I thought it would - clamps the offset into
bounds BEFORE the call to gpio_device_get_desc().

The appropriate place for this fix is in gpio_device_get_desc(), after
the bounds check.

Cheers,
Kent.

On Fri, May 17, 2024 at 12:22:39AM +0800, Kent Gibson wrote:
> On Thu, May 16, 2024 at 10:55:40PM +0800, Kent Gibson wrote:
> > On Thu, May 16, 2024 at 12:57:42PM +0000, Hagar Hemdan wrote:
> > > On Tue, May 14, 2024 at 08:42:21PM +0800, Kent Gibson wrote:
> >
> > Now I need to test your patch to see what it actually does.
> >
> 
> Tested.  Fails.  It does what I thought it would - clamps the offset into
> bounds BEFORE the call to gpio_device_get_desc().
> 
> The appropriate place for this fix is in gpio_device_get_desc(), after
> the bounds check.
> 
> Cheers,
> Kent.
>
yes, you are right. The speculation macro should be after the bounds
check. I missed this property this time.
I will fix it in v2.

Thanks,
Hagar Hemdan