linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] gpiolib: fix possible use after free on label
@ 2018-10-24 13:40 Muchun Song
  2018-10-31 10:32 ` Linus Walleij
  0 siblings, 1 reply; 3+ messages in thread
From: Muchun Song @ 2018-10-24 13:40 UTC (permalink / raw)
  To: linus.walleij; +Cc: linux-gpio, linux-kernel

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] gpiolib: fix possible use after free on label
  2018-10-24 13:40 [PATCH] gpiolib: fix possible use after free on label Muchun Song
@ 2018-10-31 10:32 ` Linus Walleij
  2018-10-31 14:25   ` Muchun Song
  0 siblings, 1 reply; 3+ messages in thread
From: Linus Walleij @ 2018-10-31 10:32 UTC (permalink / raw)
  To: smuchun; +Cc: open list:GPIO SUBSYSTEM, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] gpiolib: fix possible use after free on label
  2018-10-31 10:32 ` Linus Walleij
@ 2018-10-31 14:25   ` Muchun Song
  0 siblings, 0 replies; 3+ messages in thread
From: Muchun Song @ 2018-10-31 14:25 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-gpio, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-10-31 14:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-24 13:40 [PATCH] gpiolib: fix possible use after free on label Muchun Song
2018-10-31 10:32 ` Linus Walleij
2018-10-31 14:25   ` Muchun Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).