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 <[email protected]>
---
drivers/gpio/gpiolib.c | 25 +++++++++++++++++++++----
include/linux/gpio/consumer.h | 6 ++++--
2 files changed, 25 insertions(+), 6 deletions(-)
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)
--
2.17.1
On Thu, Nov 1, 2018 at 2:13 PM Muchun Song <[email protected]> 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 <[email protected]>
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 <[email protected]> wrote:
> On Thu, Nov 1, 2018 at 2:13 PM Muchun Song <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> wrote:
> > On Thu, Nov 1, 2018 at 2:13 PM Muchun Song <[email protected]> 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 <[email protected]>
> >
> > 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