Message ID | 20240322090209.13384-1-brgl@bgdev.pl |
---|---|
State | New |
Headers | show |
Series | [v2] gpio: cdev: sanitize the label before requesting the interrupt | expand |
On Fri, Mar 22, 2024 at 10:02:08AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > @@ -2198,12 +2216,18 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) > if (ret) > goto out_free_le; > > + label = make_irq_label(le->label); > + if (!label) { > + ret = -ENOMEM; > + goto out_free_le; > + } > + > /* Request a thread to read the events */ > ret = request_threaded_irq(irq, > lineevent_irq_handler, > lineevent_irq_thread, > irqflags, > - le->label, > + label, > le); > if (ret) > goto out_free_le; Leaks label if the request_threaded_irq() fails. Cheers, Kent.
On Fri, Mar 22, 2024 at 12:18 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Mar 22, 2024 at 10:02:08AM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > @@ -2198,12 +2216,18 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) > > if (ret) > > goto out_free_le; > > > > + label = make_irq_label(le->label); > > + if (!label) { > > + ret = -ENOMEM; > > + goto out_free_le; > > + } > > + > > /* Request a thread to read the events */ > > ret = request_threaded_irq(irq, > > lineevent_irq_handler, > > lineevent_irq_thread, > > irqflags, > > - le->label, > > + label, > > le); > > if (ret) > > goto out_free_le; > > Leaks label if the request_threaded_irq() fails. > Ah, dammit, I didn't catch the fact that irq_free() will not return the label address if the request failed. Nice catch. Bart
On Fri, Mar 22, 2024 at 12:52:29PM +0100, Bartosz Golaszewski wrote: > On Fri, Mar 22, 2024 at 12:18 PM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Fri, Mar 22, 2024 at 10:02:08AM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > @@ -2198,12 +2216,18 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) > > > if (ret) > > > goto out_free_le; > > > > > > + label = make_irq_label(le->label); > > > + if (!label) { > > > + ret = -ENOMEM; > > > + goto out_free_le; > > > + } > > > + > > > /* Request a thread to read the events */ > > > ret = request_threaded_irq(irq, > > > lineevent_irq_handler, > > > lineevent_irq_thread, > > > irqflags, > > > - le->label, > > > + label, > > > le); > > > if (ret) > > > goto out_free_le; > > > > Leaks label if the request_threaded_irq() fails. > > > > Ah, dammit, I didn't catch the fact that irq_free() will not return > the label address if the request failed. > You caught it in edge_detector_setup() - you call free_irq_label() if request_threaded_irq() fails there. > Nice catch. The give away here is that the goto targets are the same. Bit irritated that I didn't catch it earlier - got distracted by the return code problem. Anyway, third time lucky. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index f384fa278764..7a102ebac428 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -1083,10 +1083,20 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc, return 0; } +static inline char *make_irq_label(const char *orig) +{ + return kstrdup_and_replace(orig, '/', ':', GFP_KERNEL); +} + +static inline void free_irq_label(const char *label) +{ + kfree(label); +} + static void edge_detector_stop(struct line *line) { if (line->irq) { - free_irq(line->irq, line); + free_irq_label(free_irq(line->irq, line)); line->irq = 0; } @@ -1110,6 +1120,7 @@ static int edge_detector_setup(struct line *line, unsigned long irqflags = 0; u64 eflags; int irq, ret; + char *label; eflags = edflags & GPIO_V2_LINE_EDGE_FLAGS; if (eflags && !kfifo_initialized(&line->req->events)) { @@ -1146,11 +1157,17 @@ static int edge_detector_setup(struct line *line, IRQF_TRIGGER_RISING : IRQF_TRIGGER_FALLING; irqflags |= IRQF_ONESHOT; + label = make_irq_label(line->req->label); + if (!label) + return -ENOMEM; + /* Request a thread to read the events */ ret = request_threaded_irq(irq, edge_irq_handler, edge_irq_thread, - irqflags, line->req->label, line); - if (ret) + irqflags, label, line); + if (ret) { + free_irq_label(label); return ret; + } line->irq = irq; return 0; @@ -1973,7 +1990,7 @@ static void lineevent_free(struct lineevent_state *le) blocking_notifier_chain_unregister(&le->gdev->device_notifier, &le->device_unregistered_nb); if (le->irq) - free_irq(le->irq, le); + free_irq_label(free_irq(le->irq, le)); if (le->desc) gpiod_free(le->desc); kfree(le->label); @@ -2114,6 +2131,7 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) int fd; int ret; int irq, irqflags = 0; + char *label; if (copy_from_user(&eventreq, ip, sizeof(eventreq))) return -EFAULT; @@ -2198,12 +2216,18 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) if (ret) goto out_free_le; + label = make_irq_label(le->label); + if (!label) { + ret = -ENOMEM; + goto out_free_le; + } + /* Request a thread to read the events */ ret = request_threaded_irq(irq, lineevent_irq_handler, lineevent_irq_thread, irqflags, - le->label, + label, le); if (ret) goto out_free_le;