Re: [PATCH] gpio: prevent potential speculation leaks in gpio_device_get_desc()

[Kent Gibson]

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.