Received-SPF: pass (google.com: domain of linux-kernel+bounces-178664-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 14 May 2024 20:42:21 +0800
From: Kent Gibson <warthog618@gmail.com>
To: Hagar Hemdan <hagarhem@amazon.com>
Cc: Norbert Manthey <nmanthey@amazon.de>,
	Bartosz Golaszewski <brgl@bgdev.pl>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] gpio: prevent potential speculation leaks in
 gpio_device_get_desc()
Message-ID: <20240514124221.GA76024@rigel>
References: <20240514122601.15261-1-hagarhem@amazon.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240514122601.15261-1-hagarhem@amazon.com>

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
>