* [PATCH 0/2] gpio: cdev: label sanitization fixes
@ 2024-04-03 13:15 Kent Gibson
2024-04-03 13:15 ` [PATCH 1/2] gpio: cdev: fix missed label sanitizing in debounce_setup() Kent Gibson
2024-04-03 13:15 ` [PATCH 2/2] gpio: cdev: check for NULL labels when sanitizing them for irqs Kent Gibson
0 siblings, 2 replies; 6+ messages in thread
From: Kent Gibson @ 2024-04-03 13:15 UTC (permalink / raw
To: linux-kernel, linux-gpio, brgl, linus.walleij; +Cc: Kent Gibson
This series fixes a couple of bugs in the sanitization of labels
being passed to irq.
Patch 1 fixes a missed path in the sanitization changes that can result
in memory corruption.
Patch 2 fixes the case where userspace provides empty labels.
I've placed my Patch 1 before Bart's Patch 2 as it has to relocate
make_irq_label() and free_irq_label(), while Bart's patch modifies
them. This order keeps the patch sizes minimal and the attribution
where it belongs. Patch 2 has been very lightly modified to rebase it
onto Patch 1, including extending it to cover the modified error
return for the debounce_setup() case.
Cheers,
Kent.
Bartosz Golaszewski (1):
gpio: cdev: check for NULL labels when sanitizing them for irqs
Kent Gibson (1):
gpio: cdev: fix missed label sanitizing in debounce_setup()
drivers/gpio/gpiolib-cdev.c | 48 ++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 16 deletions(-)
base-commit: 782f4e47ffc19622bf80b3c0cf9cadd2b0b9a644
--
2.39.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] gpio: cdev: fix missed label sanitizing in debounce_setup()
2024-04-03 13:15 [PATCH 0/2] gpio: cdev: label sanitization fixes Kent Gibson
@ 2024-04-03 13:15 ` Kent Gibson
2024-04-04 8:20 ` Bartosz Golaszewski
2024-04-03 13:15 ` [PATCH 2/2] gpio: cdev: check for NULL labels when sanitizing them for irqs Kent Gibson
1 sibling, 1 reply; 6+ messages in thread
From: Kent Gibson @ 2024-04-03 13:15 UTC (permalink / raw
To: linux-kernel, linux-gpio, brgl, linus.walleij; +Cc: Kent Gibson, stable
When adding sanitization of the label, the path through
edge_detector_setup() that leads to debounce_setup() was overlooked.
A request taking this path does not allocate a new label and the
request label is freed twice when the request is released, resulting
in memory corruption.
Add label sanitization to debounce_setup().
Cc: stable@vger.kernel.org
Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
Signed-off-by: Kent Gibson <warthog618@gmail.com>
---
drivers/gpio/gpiolib-cdev.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index fa9635610251..f4c2da2041e5 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -728,6 +728,16 @@ static u32 line_event_id(int level)
GPIO_V2_LINE_EVENT_FALLING_EDGE;
}
+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);
+}
+
#ifdef CONFIG_HTE
static enum hte_return process_hw_ts_thread(void *p)
@@ -1015,6 +1025,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
{
unsigned long irqflags;
int ret, level, irq;
+ char *label;
/* try hardware */
ret = gpiod_set_debounce(line->desc, debounce_period_us);
@@ -1037,11 +1048,17 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
if (irq < 0)
return -ENXIO;
+ label = make_irq_label(line->req->label);
+ if (!label)
+ return -ENOMEM;
+
irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
ret = request_irq(irq, debounce_irq_handler, irqflags,
- line->req->label, line);
- if (ret)
+ label, line);
+ if (ret) {
+ free_irq_label(label);
return ret;
+ }
line->irq = irq;
} else {
ret = hte_edge_setup(line, GPIO_V2_LINE_FLAG_EDGE_BOTH);
@@ -1083,16 +1100,6 @@ 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) {
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] gpio: cdev: check for NULL labels when sanitizing them for irqs
2024-04-03 13:15 [PATCH 0/2] gpio: cdev: label sanitization fixes Kent Gibson
2024-04-03 13:15 ` [PATCH 1/2] gpio: cdev: fix missed label sanitizing in debounce_setup() Kent Gibson
@ 2024-04-03 13:15 ` Kent Gibson
1 sibling, 0 replies; 6+ messages in thread
From: Kent Gibson @ 2024-04-03 13:15 UTC (permalink / raw
To: linux-kernel, linux-gpio, brgl, linus.walleij
Cc: Bartosz Golaszewski, stable, Linux Kernel Functional Testing,
Kent Gibson
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
We need to take into account that a line's consumer label may be NULL
and not try to kstrdup() it in that case but rather pass the NULL
pointer up the stack to the interrupt request function.
To that end: let make_irq_label() return NULL as a valid return value
and use ERR_PTR() instead to signal an allocation failure to callers.
Cc: stable@vger.kernel.org
Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
Reported-by: Linux Kernel Functional Testing <lkft@linaro.org>
Closes: https://lore.kernel.org/lkml/20240402093534.212283-1-naresh.kamboju@linaro.org/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Signed-off-by: Kent Gibson <warthog618@gmail.com> (rebased)
---
drivers/gpio/gpiolib-cdev.c | 23 ++++++++++++++++-------
1 file changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index f4c2da2041e5..8112ec36e55f 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -730,7 +730,16 @@ static u32 line_event_id(int level)
static inline char *make_irq_label(const char *orig)
{
- return kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
+ char *new;
+
+ if (!orig)
+ return NULL;
+
+ new = kstrdup_and_replace(orig, '/', ':', GFP_KERNEL);
+ if (!new)
+ return ERR_PTR(-ENOMEM);
+
+ return new;
}
static inline void free_irq_label(const char *label)
@@ -1049,8 +1058,8 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
return -ENXIO;
label = make_irq_label(line->req->label);
- if (!label)
- return -ENOMEM;
+ if (IS_ERR(label))
+ return PTR_ERR(label);
irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
ret = request_irq(irq, debounce_irq_handler, irqflags,
@@ -1165,8 +1174,8 @@ static int edge_detector_setup(struct line *line,
irqflags |= IRQF_ONESHOT;
label = make_irq_label(line->req->label);
- if (!label)
- return -ENOMEM;
+ if (IS_ERR(label))
+ return PTR_ERR(label);
/* Request a thread to read the events */
ret = request_threaded_irq(irq, edge_irq_handler, edge_irq_thread,
@@ -2224,8 +2233,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
goto out_free_le;
label = make_irq_label(le->label);
- if (!label) {
- ret = -ENOMEM;
+ if (IS_ERR(label)) {
+ ret = PTR_ERR(label);
goto out_free_le;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gpio: cdev: fix missed label sanitizing in debounce_setup()
2024-04-03 13:15 ` [PATCH 1/2] gpio: cdev: fix missed label sanitizing in debounce_setup() Kent Gibson
@ 2024-04-04 8:20 ` Bartosz Golaszewski
2024-04-04 10:59 ` Kent Gibson
0 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2024-04-04 8:20 UTC (permalink / raw
To: Kent Gibson; +Cc: linux-kernel, linux-gpio, linus.walleij, stable
On Wed, Apr 3, 2024 at 3:15 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> When adding sanitization of the label, the path through
> edge_detector_setup() that leads to debounce_setup() was overlooked.
> A request taking this path does not allocate a new label and the
> request label is freed twice when the request is released, resulting
> in memory corruption.
>
> Add label sanitization to debounce_setup().
>
> Cc: stable@vger.kernel.org
> Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
> Signed-off-by: Kent Gibson <warthog618@gmail.com>
> ---
> drivers/gpio/gpiolib-cdev.c | 31 +++++++++++++++++++------------
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index fa9635610251..f4c2da2041e5 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -728,6 +728,16 @@ static u32 line_event_id(int level)
> GPIO_V2_LINE_EVENT_FALLING_EDGE;
> }
>
> +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);
> +}
> +
> #ifdef CONFIG_HTE
>
> static enum hte_return process_hw_ts_thread(void *p)
> @@ -1015,6 +1025,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
> {
> unsigned long irqflags;
> int ret, level, irq;
> + char *label;
>
> /* try hardware */
> ret = gpiod_set_debounce(line->desc, debounce_period_us);
> @@ -1037,11 +1048,17 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
> if (irq < 0)
> return -ENXIO;
>
> + label = make_irq_label(line->req->label);
Now that I look at the actual patch, I don't really like it. We
introduce a bug just to fix it a commit later. Such things have been
frowned upon in the past.
Let me shuffle the code a bit, I'll try to make it a bit more correct.
Bart
> + if (!label)
> + return -ENOMEM;
> +
> irqflags = IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING;
> ret = request_irq(irq, debounce_irq_handler, irqflags,
> - line->req->label, line);
> - if (ret)
> + label, line);
> + if (ret) {
> + free_irq_label(label);
> return ret;
> + }
> line->irq = irq;
> } else {
> ret = hte_edge_setup(line, GPIO_V2_LINE_FLAG_EDGE_BOTH);
> @@ -1083,16 +1100,6 @@ 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) {
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gpio: cdev: fix missed label sanitizing in debounce_setup()
2024-04-04 8:20 ` Bartosz Golaszewski
@ 2024-04-04 10:59 ` Kent Gibson
2024-04-04 12:20 ` Bartosz Golaszewski
0 siblings, 1 reply; 6+ messages in thread
From: Kent Gibson @ 2024-04-04 10:59 UTC (permalink / raw
To: Bartosz Golaszewski; +Cc: linux-kernel, linux-gpio, linus.walleij, stable
On Thu, Apr 04, 2024 at 10:20:29AM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 3, 2024 at 3:15 PM Kent Gibson <warthog618@gmail.com> wrote:
> >
> > When adding sanitization of the label, the path through
> > edge_detector_setup() that leads to debounce_setup() was overlooked.
> > A request taking this path does not allocate a new label and the
> > request label is freed twice when the request is released, resulting
> > in memory corruption.
> >
> > Add label sanitization to debounce_setup().
> >
> > Cc: stable@vger.kernel.org
> > Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
> > Signed-off-by: Kent Gibson <warthog618@gmail.com>
> > ---
> > drivers/gpio/gpiolib-cdev.c | 31 +++++++++++++++++++------------
> > 1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > index fa9635610251..f4c2da2041e5 100644
> > --- a/drivers/gpio/gpiolib-cdev.c
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -728,6 +728,16 @@ static u32 line_event_id(int level)
> > GPIO_V2_LINE_EVENT_FALLING_EDGE;
> > }
> >
> > +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);
> > +}
> > +
> > #ifdef CONFIG_HTE
> >
> > static enum hte_return process_hw_ts_thread(void *p)
> > @@ -1015,6 +1025,7 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
> > {
> > unsigned long irqflags;
> > int ret, level, irq;
> > + char *label;
> >
> > /* try hardware */
> > ret = gpiod_set_debounce(line->desc, debounce_period_us);
> > @@ -1037,11 +1048,17 @@ static int debounce_setup(struct line *line, unsigned int debounce_period_us)
> > if (irq < 0)
> > return -ENXIO;
> >
> > + label = make_irq_label(line->req->label);
>
> Now that I look at the actual patch, I don't really like it. We
> introduce a bug just to fix it a commit later. Such things have been
> frowned upon in the past.
>
> Let me shuffle the code a bit, I'll try to make it a bit more correct.
>
The debounce_setup() oversight bug is the more severe, so it makes more
sense to me to fix it first. But then I my preferred solution would be
to pull the original patch and submit a corrected patch that merges all
three, so no bugs, but I assume that isn't an option.
Cheers,
Kent.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] gpio: cdev: fix missed label sanitizing in debounce_setup()
2024-04-04 10:59 ` Kent Gibson
@ 2024-04-04 12:20 ` Bartosz Golaszewski
0 siblings, 0 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2024-04-04 12:20 UTC (permalink / raw
To: Kent Gibson; +Cc: linux-kernel, linux-gpio, linus.walleij, stable
On Thu, Apr 4, 2024 at 12:59 PM Kent Gibson <warthog618@gmail.com> wrote:
>
> On Thu, Apr 04, 2024 at 10:20:29AM +0200, Bartosz Golaszewski wrote:
> >
> > Now that I look at the actual patch, I don't really like it. We
> > introduce a bug just to fix it a commit later. Such things have been
> > frowned upon in the past.
> >
> > Let me shuffle the code a bit, I'll try to make it a bit more correct.
> >
>
> The debounce_setup() oversight bug is the more severe, so it makes more
> sense to me to fix it first. But then I my preferred solution would be
> to pull the original patch and submit a corrected patch that merges all
> three, so no bugs, but I assume that isn't an option.
>
Nah, let's not needlessly rebase it. Most I can do is merge the two
but they are really functionally separate so I'd keep it as is in v2.
Bart
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-04-04 12:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-03 13:15 [PATCH 0/2] gpio: cdev: label sanitization fixes Kent Gibson
2024-04-03 13:15 ` [PATCH 1/2] gpio: cdev: fix missed label sanitizing in debounce_setup() Kent Gibson
2024-04-04 8:20 ` Bartosz Golaszewski
2024-04-04 10:59 ` Kent Gibson
2024-04-04 12:20 ` Bartosz Golaszewski
2024-04-03 13:15 ` [PATCH 2/2] gpio: cdev: check for NULL labels when sanitizing them for irqs Kent Gibson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).