Message ID | 20181024134040.115413-1-smuchun@gmail.com |
---|---|
State | New |
Headers | show |
Series | gpiolib: fix possible use after free on label | expand |
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
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
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);
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(+)