| Message ID | 20181101131250.41636-1-smuchun@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] gpiolib: Fix possible use after free on label | expand |
On Thu, Nov 1, 2018 at 2:13 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. > > Also fix gpiod_set_consumer_name(). > > Signed-off-by: Muchun Song <smuchun@gmail.com> I am still a bit worried about the kstrdup_const() that this introduces. The tinyfication people will not like that we now copy every GPIO line name from the device tree into a new reference copy. What we *REALLY* want to do is: const char *str; const char *ref; if (pointer_on_stack(str)) ref = kstrdup_const(str); else ref = str; Isn't this possible to achieve somehow? If not, why not? I suspect maybe there is no simple solution to this, but what about a really complicated and hard solution? I'm looping in Nico for advice. Maybe I will end up applying it anyway but I'm not sure. The patch looks good otherwise. Yours, Linus Walleij
On Thu, Nov 1, 2018 at 4:27 PM Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Nov 1, 2018 at 2:13 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. > > > > Also fix gpiod_set_consumer_name(). > > > > Signed-off-by: Muchun Song <smuchun@gmail.com> > > I am still a bit worried about the kstrdup_const() that this > introduces. Forget it. I realized after actually reading the code for kstrdup_const() that it really does exactly what we want. I should stop assuming things are syntactic sugar in the kernel, we have some really smart people working with it... Patch applied. Yours, Linus Walleij
On Fri, 2 Nov 2018, Linus Walleij wrote: > On Thu, Nov 1, 2018 at 4:27 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Nov 1, 2018 at 2:13 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. > > > > > > Also fix gpiod_set_consumer_name(). > > > > > > Signed-off-by: Muchun Song <smuchun@gmail.com> > > > > I am still a bit worried about the kstrdup_const() that this > > introduces. > > Forget it. I realized after actually reading the code > for kstrdup_const() that it really does exactly > what we want. > > I should stop assuming things are syntactic sugar > in the kernel, we have some really smart people > working with it... I didn't know about kstrdup_const() either before just now. If the device tree lands in the kernel rodata area then all is fine. I don't know enough about the actual DT processing to be sure though. Nicolas
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 25187403e3ac..550918268549 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); @@ -3212,11 +3221,19 @@ EXPORT_SYMBOL_GPL(gpiod_cansleep); * @desc: gpio to set the consumer name on * @name: the new consumer name */ -void gpiod_set_consumer_name(struct gpio_desc *desc, const char *name) +int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name) { - VALIDATE_DESC_VOID(desc); - /* Just overwrite whatever the previous name was */ - desc->label = name; + VALIDATE_DESC(desc); + if (name) { + name = kstrdup_const(name, GFP_KERNEL); + if (!name) + return -ENOMEM; + } + + kfree_const(desc->label); + desc_set_label(desc, name); + + return 0; } EXPORT_SYMBOL_GPL(gpiod_set_consumer_name); diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h index 21ddbe440030..acc4279ad5e3 100644 --- a/include/linux/gpio/consumer.h +++ b/include/linux/gpio/consumer.h @@ -142,7 +142,7 @@ int gpiod_is_active_low(const struct gpio_desc *desc); int gpiod_cansleep(const struct gpio_desc *desc); int gpiod_to_irq(const struct gpio_desc *desc); -void gpiod_set_consumer_name(struct gpio_desc *desc, const char *name); +int gpiod_set_consumer_name(struct gpio_desc *desc, const char *name); /* Convert between the old gpio_ and new gpiod_ interfaces */ struct gpio_desc *gpio_to_desc(unsigned gpio); @@ -465,10 +465,12 @@ static inline int gpiod_to_irq(const struct gpio_desc *desc) return -EINVAL; } -static inline void gpiod_set_consumer_name(struct gpio_desc *desc, const char *name) +static inline int gpiod_set_consumer_name(struct gpio_desc *desc, + const char *name) { /* GPIO can never have been requested */ WARN_ON(1); + return -EINVAL; } static inline struct gpio_desc *gpio_to_desc(unsigned gpio)
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. Also fix gpiod_set_consumer_name(). Signed-off-by: Muchun Song <smuchun@gmail.com> --- drivers/gpio/gpiolib.c | 25 +++++++++++++++++++++---- include/linux/gpio/consumer.h | 6 ++++-- 2 files changed, 25 insertions(+), 6 deletions(-)