From: Bartosz Golaszewski <[email protected]>
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: [email protected]
Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
Reported-by: Linux Kernel Functional Testing <[email protected]>
Closes: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/gpio/gpiolib-cdev.c | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
index fa9635610251..1426cc1c4a28 100644
--- a/drivers/gpio/gpiolib-cdev.c
+++ b/drivers/gpio/gpiolib-cdev.c
@@ -1085,7 +1085,16 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
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)
@@ -1158,8 +1167,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,
@@ -2217,8 +2226,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.40.1
On Tue, Apr 02, 2024 at 01:41:59PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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: [email protected]
> Fixes: b34490879baa ("gpio: cdev: sanitize the label before requesting the interrupt")
> Reported-by: Linux Kernel Functional Testing <[email protected]>
> Closes: https://lore.kernel.org/lkml/[email protected]/
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/gpio/gpiolib-cdev.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> index fa9635610251..1426cc1c4a28 100644
> --- a/drivers/gpio/gpiolib-cdev.c
> +++ b/drivers/gpio/gpiolib-cdev.c
> @@ -1085,7 +1085,16 @@ static u32 gpio_v2_line_config_debounce_period(struct gpio_v2_line_config *lc,
>
> 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)
> @@ -1158,8 +1167,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,
> @@ -2217,8 +2226,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;
> }
>
It occurred to me that none of my tests cover this case, as they always
request edges with the consumer set, so I added some and can confirm both
the problem and the fix.
In the process I found another bug - we overlooked setting up the irq
label in debounce_setup() - the alternate path in edge_detector_setup()
that performs sw debounce. That results in a double free of the
req->label and memory corruption hilarity follows.
I've got a patch for that - the unfortunate part being that
debounce_setup() is earlier in the file than make_irq_label() and
free_irq_label(). Those will need to be pushed earlier, so it is
sure to conflict with this patch.
How would you prefer to proceed?
Cheers,
Kent.
On Wed, Apr 03, 2024 at 11:47:21AM +0200, Bartosz Golaszewski wrote:
> On Wed, Apr 3, 2024 at 11:42 AM Kent Gibson <[email protected]> wrote:
> >
> >
> > It occurred to me that none of my tests cover this case, as they always
> > request edges with the consumer set, so I added some and can confirm both
> > the problem and the fix.
> >
> > In the process I found another bug - we overlooked setting up the irq
> > label in debounce_setup() - the alternate path in edge_detector_setup()
> > that performs sw debounce. That results in a double free of the
> > req->label and memory corruption hilarity follows.
> >
> > I've got a patch for that - the unfortunate part being that
> > debounce_setup() is earlier in the file than make_irq_label() and
> > free_irq_label(). Those will need to be pushed earlier, so it is
> > sure to conflict with this patch.
> > How would you prefer to proceed?
>
> Can you take my patch and just make it part of your series?
>
Will do.
Cheers,
Kent.
On Wed, Apr 3, 2024 at 11:42 AM Kent Gibson <[email protected]> wrote:
>
>
> It occurred to me that none of my tests cover this case, as they always
> request edges with the consumer set, so I added some and can confirm both
> the problem and the fix.
>
> In the process I found another bug - we overlooked setting up the irq
> label in debounce_setup() - the alternate path in edge_detector_setup()
> that performs sw debounce. That results in a double free of the
> req->label and memory corruption hilarity follows.
>
> I've got a patch for that - the unfortunate part being that
> debounce_setup() is earlier in the file than make_irq_label() and
> free_irq_label(). Those will need to be pushed earlier, so it is
> sure to conflict with this patch.
> How would you prefer to proceed?
Can you take my patch and just make it part of your series?
Bart