[PATCH] gpiolib: fix possible use after free on label

[PATCH] gpiolib: fix possible use after free on label

[Muchun Song]

gpiod_request_commit() copies the pointer to the label
passed as an argument only to be used later. But there's a
chance the caller could immediately free the passed string
(e.g., local variable). This could trigger a use after free
when we use gpio label(e.g., gpiochip_unlock_as_irq(),
gpiochip_is_requested()).

To be on the safe side: duplicate the string with
kstrdup_const() so that if an unaware user passes an address
to a stack-allocated buffer, we won't get the arbitrary label.

Signed-off-by: Muchun Song <smuchun@gmail.com>
---
 drivers/gpio/gpiolib.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 25187403e3ac..e600c5f5d9a7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2270,6 +2270,12 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 	unsigned long		flags;
 	unsigned		offset;
 
+	if (label) {
+		label = kstrdup_const(label, GFP_KERNEL);
+		if (!label)
+			return -ENOMEM;
+	}
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	/* NOTE:  gpio_request() can be called in early boot,
@@ -2280,6 +2286,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 		desc_set_label(desc, label ? : "?");
 		status = 0;
 	} else {
+		kfree_const(label);
 		status = -EBUSY;
 		goto done;
 	}
@@ -2296,6 +2303,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
 
 		if (status < 0) {
 			desc_set_label(desc, NULL);
+			kfree_const(label);
 			clear_bit(FLAG_REQUESTED, &desc->flags);
 			goto done;
 		}
@@ -2391,6 +2399,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
 			chip->free(chip, gpio_chip_hwgpio(desc));
 			spin_lock_irqsave(&gpio_lock, flags);
 		}
+		kfree_const(desc->label);
 		desc_set_label(desc, NULL);
 		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
 		clear_bit(FLAG_REQUESTED, &desc->flags);
-- 
2.17.1

Re: [PATCH] gpiolib: fix possible use after free on label

[Linus Walleij]

Hi Muchun,

thanks for your patch!

On Wed, Oct 24, 2018 at 3:41 PM Muchun Song <smuchun@gmail.com> wrote:

> gpiod_request_commit() copies the pointer to the label
> passed as an argument only to be used later. But there's a
> chance the caller could immediately free the passed string
> (e.g., local variable). This could trigger a use after free
> when we use gpio label(e.g., gpiochip_unlock_as_irq(),
> gpiochip_is_requested()).
>
> To be on the safe side: duplicate the string with
> kstrdup_const() so that if an unaware user passes an address
> to a stack-allocated buffer, we won't get the arbitrary label.
>
> Signed-off-by: Muchun Song <smuchun@gmail.com>

I am a bit worried about the memory consumption for this,
but I guess typically this should not be much.

I am a little bit lost in const-correctness here, and I do
understand that the label could point to something allocated on
the stack, but it seems like an awkward way of shooting
oneself in the foot really. Allocate something and then
pass it as a const char *? That is something we could pretty
much detect with a cocinelle script I think?

Anyways: if you want to proceed with this approach, also
make sure to fix gpiod_set_consumer_name() to free previous
label and make a new strdup when called.

Yours,
Linus Walleij

Re: [PATCH] gpiolib: fix possible use after free on label

[Muchun Song]

Hi Linus,

Thanks for your review.

Linus Walleij <linus.walleij@linaro.org> 于2018年10月31日周三 下午6:32写道：
>
> Hi Muchun,
>
> thanks for your patch!
>
> On Wed, Oct 24, 2018 at 3:41 PM Muchun Song <smuchun@gmail.com> wrote:
>
> > gpiod_request_commit() copies the pointer to the label
> > passed as an argument only to be used later. But there's a
> > chance the caller could immediately free the passed string
> > (e.g., local variable). This could trigger a use after free
> > when we use gpio label(e.g., gpiochip_unlock_as_irq(),
> > gpiochip_is_requested()).
> >
> > To be on the safe side: duplicate the string with
> > kstrdup_const() so that if an unaware user passes an address
> > to a stack-allocated buffer, we won't get the arbitrary label.
> >
> > Signed-off-by: Muchun Song <smuchun@gmail.com>
>
> I am a bit worried about the memory consumption for this,
> but I guess typically this should not be much.

Yeah, I think so. In most cases, we pass the label, which is
in .rodata section.

>
> I am a little bit lost in const-correctness here, and I do
> understand that the label could point to something allocated on
> the stack, but it seems like an awkward way of shooting
> oneself in the foot really. Allocate something and then
> pass it as a const char *? That is something we could pretty
> much detect with a cocinelle script I think?

Some user may have more than one gpio to request
and may program the following code to request one gpio：

int gpio_request_one(int gpio)
{
        char name[8];

        snprintf(name, sizeof(name), "GPIO_%d", gpio);

        return gpio_request(gpio, name);
}

In this case, it could trigger a use after free when we use gpio label.
But the user may not realize it. With this patch applied, we can get
the right label.

>
> Anyways: if you want to proceed with this approach, also
> make sure to fix gpiod_set_consumer_name() to free previous
> label and make a new strdup when called.
>
> Yours,
> Linus Walleij

Sorry, I forgot to fix gpiod_set_consumer_name().
I will send you a patch of v2 later. Thanks.

Yours,
Muchun Song