* [patch] gpiolib: potential oops on failure path
@ 2016-06-17 9:15 Dan Carpenter
2016-06-17 9:28 ` walter harms
2016-06-18 8:52 ` Linus Walleij
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2016-06-17 9:15 UTC (permalink / raw)
To: Linus Walleij; +Cc: Alexandre Courbot, linux-gpio, kernel-janitors
If anon_inode_getfd() fails then "i" is set to GPIOHANDLES_MAX. It
means that we will read beyond the end of the array and dereference an
invalid pointer.
Fixes: d7c51b47ac11 ('gpio: userspace ABI for reading/writing GPIO lines')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 8b3db59..8578b7f 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -495,6 +495,8 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
return 0;
out_free_descs:
+ if (i = GPIOHANDLES_MAX)
+ i--;
for (; i >= 0; i--)
gpiod_free(lh->descs[i]);
kfree(lh->label);
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [patch] gpiolib: potential oops on failure path
2016-06-17 9:15 [patch] gpiolib: potential oops on failure path Dan Carpenter
@ 2016-06-17 9:28 ` walter harms
2016-06-17 9:59 ` Dan Carpenter
2016-06-18 8:54 ` Linus Walleij
2016-06-18 8:52 ` Linus Walleij
1 sibling, 2 replies; 6+ messages in thread
From: walter harms @ 2016-06-17 9:28 UTC (permalink / raw)
To: Dan Carpenter
Cc: Linus Walleij, Alexandre Courbot, linux-gpio, kernel-janitors
Am 17.06.2016 11:15, schrieb Dan Carpenter:
> If anon_inode_getfd() fails then "i" is set to GPIOHANDLES_MAX. It
> means that we will read beyond the end of the array and dereference an
> invalid pointer.
>
> Fixes: d7c51b47ac11 ('gpio: userspace ABI for reading/writing GPIO lines')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 8b3db59..8578b7f 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -495,6 +495,8 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
> return 0;
>
> out_free_descs:
> + if (i = GPIOHANDLES_MAX)
> + i--;
> for (; i >= 0; i--)
> gpiod_free(lh->descs[i]);
> kfree(lh->label);
Since we have already noticed that programmes are bad at counting backwards
is it possible to change the loop into counting up ?
btw: if lh->descs[i] is initialized to NULL it would be more robust just to free everything like:
for(i=0;i< GPIOHANDLES_MAX; i++)
gpiod_free(lh->descs[i]);
just my two cents,
re,
wh
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] gpiolib: potential oops on failure path
2016-06-17 9:28 ` walter harms
@ 2016-06-17 9:59 ` Dan Carpenter
2016-06-18 8:54 ` Linus Walleij
1 sibling, 0 replies; 6+ messages in thread
From: Dan Carpenter @ 2016-06-17 9:59 UTC (permalink / raw)
To: walter harms
Cc: Linus Walleij, Alexandre Courbot, linux-gpio, kernel-janitors
On Fri, Jun 17, 2016 at 11:28:33AM +0200, walter harms wrote:
> > out_free_descs:
> > + if (i = GPIOHANDLES_MAX)
> > + i--;
> > for (; i >= 0; i--)
> > gpiod_free(lh->descs[i]);
> > kfree(lh->label);
>
>
> Since we have already noticed that programmes are bad at counting backwards
> is it possible to change the loop into counting up ?
>
> btw: if lh->descs[i] is initialized to NULL it would be more robust just to free everything like:
>
> for(i=0;i< GPIOHANDLES_MAX; i++)
> gpiod_free(lh->descs[i]);
>
Depending on the config gpiod_free(NULL) generates a warning. It's
possible that it will be generated once with the current code actually
but I didn't feel it was worth worrying about that.
Still I wouldn't want to generate 64 warnings.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] gpiolib: potential oops on failure path
2016-06-17 9:28 ` walter harms
2016-06-17 9:59 ` Dan Carpenter
@ 2016-06-18 8:54 ` Linus Walleij
1 sibling, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2016-06-18 8:54 UTC (permalink / raw)
To: wharms
Cc: Dan Carpenter, Alexandre Courbot, linux-gpio@vger.kernel.org,
kernel-janitors
On Fri, Jun 17, 2016 at 11:28 AM, walter harms <wharms@bfs.de> wrote:
> Am 17.06.2016 11:15, schrieb Dan Carpenter:
>> out_free_descs:
>> + if (i = GPIOHANDLES_MAX)
>> + i--;
>> for (; i >= 0; i--)
>> gpiod_free(lh->descs[i]);
>> kfree(lh->label);
>
>
> Since we have already noticed that programmes are bad at counting backwards
> is it possible to change the loop into counting up ?
>
> btw: if lh->descs[i] is initialized to NULL it would be more robust just to free everything like:
>
> for(i=0;i< GPIOHANDLES_MAX; i++)
> gpiod_free(lh->descs[i]);
>
> just my two cents,
The loop looks like that for a reason: i is indexing upwards while
grabbing the descriptors, then it rewinds that way to free them if
something went wrong during init.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] gpiolib: potential oops on failure path
2016-06-17 9:15 [patch] gpiolib: potential oops on failure path Dan Carpenter
2016-06-17 9:28 ` walter harms
@ 2016-06-18 8:52 ` Linus Walleij
2016-06-18 8:57 ` Linus Walleij
1 sibling, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2016-06-18 8:52 UTC (permalink / raw)
To: Dan Carpenter
Cc: Alexandre Courbot, linux-gpio@vger.kernel.org, kernel-janitors
On Fri, Jun 17, 2016 at 11:15 AM, Dan Carpenter
<dan.carpenter@oracle.com> wrote:
> If anon_inode_getfd() fails then "i" is set to GPIOHANDLES_MAX. It
> means that we will read beyond the end of the array and dereference an
> invalid pointer.
>
> Fixes: d7c51b47ac11 ('gpio: userspace ABI for reading/writing GPIO lines')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Ouch, yeah that happens when you request max number of handles, thanks.
Patch applied.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [patch] gpiolib: potential oops on failure path
2016-06-18 8:52 ` Linus Walleij
@ 2016-06-18 8:57 ` Linus Walleij
0 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2016-06-18 8:57 UTC (permalink / raw)
To: Dan Carpenter
Cc: Alexandre Courbot, linux-gpio@vger.kernel.org, kernel-janitors
On Sat, Jun 18, 2016 at 10:52 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Fri, Jun 17, 2016 at 11:15 AM, Dan Carpenter
> <dan.carpenter@oracle.com> wrote:
>
>> If anon_inode_getfd() fails then "i" is set to GPIOHANDLES_MAX. It
>> means that we will read beyond the end of the array and dereference an
>> invalid pointer.
>>
>> Fixes: d7c51b47ac11 ('gpio: userspace ABI for reading/writing GPIO lines')
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Ouch, yeah that happens when you request max number of handles, thanks.
>
> Patch applied.
Ah no, I saw the real problem now. Sending another patch.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-18 8:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-17 9:15 [patch] gpiolib: potential oops on failure path Dan Carpenter
2016-06-17 9:28 ` walter harms
2016-06-17 9:59 ` Dan Carpenter
2016-06-18 8:54 ` Linus Walleij
2016-06-18 8:52 ` Linus Walleij
2016-06-18 8:57 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox