Message ID | 20240320125945.16985-1-brgl@bgdev.pl |
---|---|
State | New |
Headers | show |
Series | gpio: cdev: sanitize the label before requesting the interrupt | expand |
On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > When an interrupt is requested, a procfs directory is created under > "/proc/irq/<irqnum>/<label>" where <label> is the string passed to one of > the request_irq() variants. > > What follows is that the string must not contain the "/" character or > the procfs mkdir operation will fail. We don't have such constraints for > GPIO consumer labels which are used verbatim as interrupt labels for > GPIO irqs. We must therefore sanitize the consumer string before > requesting the interrupt. > As previously mentioned, it would be nice for that constraint to be documented in the irq header to help prevent others falling into the same trap. Else a short comment in the code here to explain the need for sanitization. > Let's replace all "/" with "-". > I actually prefer the ":" you originally suggested, as it more clearly indicates a tier separation, whereas a hyphen is commonly used for multi-word names. And as the hyphen is more commonly used the sanitized name is more likely to conflict. > Cc: stable@vger.kernel.org > Reported-by: Stefan Wahren <wahrenst@gmx.net> > Closes: https://lore.kernel.org/linux-gpio/39fe95cb-aa83-4b8b-8cab-63947a726754@gmx.net/ > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/gpio/gpiolib-cdev.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c > index f384fa278764..8b5e8e92cbb5 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,16 @@ 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) > + goto out_free_le; > + Need to set ret = -ENOMEM before the goto, else you will return 0. Cheers, Kent. > /* 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; > -- > 2.40.1 >
On Fri, Mar 22, 2024 at 2:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Let's replace all "/" with "-". > > > > I actually prefer the ":" you originally suggested, as it more clearly > indicates a tier separation, whereas a hyphen is commonly used for > multi-word names. And as the hyphen is more commonly used the sanitized > name is more likely to conflict. > Sounds good, will do. > > > > + label = make_irq_label(le->label); > > + if (!label) > > + goto out_free_le; > > + > > Need to set ret = -ENOMEM before the goto, else you will return 0. > Eek, right, thanks. Bart
On Fri, Mar 22, 2024 at 08:46:50AM +0100, Bartosz Golaszewski wrote: > On Fri, Mar 22, 2024 at 2:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Let's replace all "/" with "-". > > > > > > > I actually prefer the ":" you originally suggested, as it more clearly > > indicates a tier separation, whereas a hyphen is commonly used for > > multi-word names. And as the hyphen is more commonly used the sanitized > > name is more likely to conflict. > > > > Sounds good, will do. > > > > > > + label = make_irq_label(le->label); > > > + if (!label) > > > + goto out_free_le; > > > + > > > > Need to set ret = -ENOMEM before the goto, else you will return 0. > > > > Eek, right, thanks. Smatch has a warning about this, btw. drivers/gpio/gpiolib-cdev.c:2221 lineevent_create() warn: missing error code 'ret' The other warning here is: drivers/gpio/gpiolib-cdev.c:2269 lineevent_create() warn: 'irq' from request_threaded_irq() not released on lines: 2258. regards, dan carpenter
On Fri, Mar 22, 2024 at 12:31:36PM +0300, Dan Carpenter wrote: > On Fri, Mar 22, 2024 at 08:46:50AM +0100, Bartosz Golaszewski wrote: > > On Fri, Mar 22, 2024 at 2:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > Let's replace all "/" with "-". > > > > > > > > > > I actually prefer the ":" you originally suggested, as it more clearly > > > indicates a tier separation, whereas a hyphen is commonly used for > > > multi-word names. And as the hyphen is more commonly used the sanitized > > > name is more likely to conflict. > > > > > > > Sounds good, will do. > > > > > > > > + label = make_irq_label(le->label); > > > > + if (!label) > > > > + goto out_free_le; > > > > + > > > > > > Need to set ret = -ENOMEM before the goto, else you will return 0. > > > > > > > Eek, right, thanks. > > Smatch has a warning about this, btw. > drivers/gpio/gpiolib-cdev.c:2221 lineevent_create() warn: missing error code 'ret' > And that triggered a "what the hell does that mean" warning in my wetware error parser ;-). That could be better worded - it isn't "missing", it hasn't been appropriately set. So maybe "unset error code"? > The other warning here is: > drivers/gpio/gpiolib-cdev.c:2269 lineevent_create() warn: 'irq' from request_threaded_irq() not released on lines: 2258. > Looks like a false positive to me - as per the comment in the code, that path (2258) results in lineevent_release() being called and that releases the irq. Cheers, Kent.
On Fri, Mar 22, 2024 at 07:54:19PM +0800, Kent Gibson wrote: > On Fri, Mar 22, 2024 at 12:31:36PM +0300, Dan Carpenter wrote: > > On Fri, Mar 22, 2024 at 08:46:50AM +0100, Bartosz Golaszewski wrote: > > > On Fri, Mar 22, 2024 at 2:30 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > On Wed, Mar 20, 2024 at 01:59:44PM +0100, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > > > Let's replace all "/" with "-". > > > > > > > > > > > > > I actually prefer the ":" you originally suggested, as it more clearly > > > > indicates a tier separation, whereas a hyphen is commonly used for > > > > multi-word names. And as the hyphen is more commonly used the sanitized > > > > name is more likely to conflict. > > > > > > > > > > Sounds good, will do. > > > > > > > > > > + label = make_irq_label(le->label); > > > > > + if (!label) > > > > > + goto out_free_le; > > > > > + > > > > > > > > Need to set ret = -ENOMEM before the goto, else you will return 0. > > > > > > > > > > Eek, right, thanks. > > > > Smatch has a warning about this, btw. > > drivers/gpio/gpiolib-cdev.c:2221 lineevent_create() warn: missing error code 'ret' > > > > And that triggered a "what the hell does that mean" warning in my > wetware error parser ;-). > > That could be better worded - it isn't "missing", it hasn't been > appropriately set. So maybe "unset error code"? > It's kind of a pain to change the warning message after the fact because then they show up as new warnings for everyone... I maybe should poll people to see if they care about the hassel of it. > > The other warning here is: > > drivers/gpio/gpiolib-cdev.c:2269 lineevent_create() warn: 'irq' from request_threaded_irq() not released on lines: 2258. > > > > Looks like a false positive to me - as per the comment in the code, that path > (2258) results in lineevent_release() being called and that releases the irq. Ah right. Thanks! regards, dan carpenter
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index f384fa278764..8b5e8e92cbb5 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,16 @@ 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) + 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;