* [PATCH] gpiolib: check if gpio_desc pointer is error or NULL
@ 2014-03-18 10:41 Ben Dooks
2014-03-19 1:48 ` Alexandre Courbot
0 siblings, 1 reply; 4+ messages in thread
From: Ben Dooks @ 2014-03-18 10:41 UTC (permalink / raw)
To: linux-kernel
Cc: Ben Dooks, Linus Walleij, Alexandre Courbot, linux-gpio,
linux-kernel
Some of the gpiod_ calls take a pointer to a gpio_desc as their
argument but only check to see if it is NULL to validate the
input.
Calls such as devm_gpiod_get() return an error-pointer if they
fail, so doing the following will not work:
gpio = devm_gpiod_get(...);
gpiod_direction_output(gpio, 0);
The sequence produces an OOPS like:
Unable to handle kernel paging request at virtual address fffffffe
Change all calls that check for !desc to use IS_ERR_OR_NULL() to
avoid these issues.
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: linux-gpio@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
drivers/gpio/gpiolib.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 50c4922..e69ac5e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -817,7 +817,7 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change)
return -ENOENT;
}
- if (!desc) {
+ if (IS_ERR_OR_NULL(desc)) {
pr_debug("%s: invalid gpio descriptor\n", __func__);
return -EINVAL;
}
@@ -903,7 +903,7 @@ int gpiod_export_link(struct device *dev, const char *name,
{
int status = -EINVAL;
- if (!desc) {
+ if (IS_ERR_OR_NULL(desc)) {
pr_warn("%s: invalid GPIO\n", __func__);
return -EINVAL;
}
@@ -986,7 +986,7 @@ void gpiod_unexport(struct gpio_desc *desc)
int status = 0;
struct device *dev = NULL;
- if (!desc) {
+ if (IS_ERR_OR_NULL(desc)) {
pr_warn("%s: invalid GPIO\n", __func__);
return;
}
@@ -1463,7 +1463,7 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
int status = -EPROBE_DEFER;
unsigned long flags;
- if (!desc) {
+ if (IS_ERR_OR_NULL(desc)) {
pr_warn("%s: invalid GPIO\n", __func__);
return -EINVAL;
}
@@ -1529,7 +1529,7 @@ static void gpiod_free(struct gpio_desc *desc)
might_sleep();
- if (!desc) {
+ if (IS_ERR_OR_NULL(desc)) {
WARN_ON(extra_checks);
return;
}
@@ -1703,7 +1703,7 @@ int gpiod_direction_input(struct gpio_desc *desc)
int status = -EINVAL;
int offset;
- if (!desc || !desc->chip) {
+ if (IS_ERR_OR_NULL(desc) || !desc->chip) {
pr_warn("%s: invalid GPIO\n", __func__);
return -EINVAL;
}
@@ -1773,7 +1773,7 @@ int gpiod_direction_output(struct gpio_desc *desc, int value)
int status = -EINVAL;
int offset;
- if (!desc || !desc->chip) {
+ if (IS_ERR_OR_NULL(desc) || !desc->chip) {
pr_warn("%s: invalid GPIO\n", __func__);
return -EINVAL;
}
@@ -1857,7 +1857,7 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce)
int status = -EINVAL;
int offset;
- if (!desc || !desc->chip) {
+ if (IS_ERR_OR_NULL(desc) || !desc->chip) {
pr_warn("%s: invalid GPIO\n", __func__);
return -EINVAL;
}
@@ -1953,7 +1953,7 @@ static int _gpiod_get_raw_value(const struct gpio_desc *desc)
*/
int gpiod_get_raw_value(const struct gpio_desc *desc)
{
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return 0;
/* Should be using gpio_get_value_cansleep() */
WARN_ON(desc->chip->can_sleep);
@@ -1974,7 +1974,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_raw_value);
int gpiod_get_value(const struct gpio_desc *desc)
{
int value;
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return 0;
/* Should be using gpio_get_value_cansleep() */
WARN_ON(desc->chip->can_sleep);
@@ -2068,7 +2068,7 @@ static void _gpiod_set_raw_value(struct gpio_desc *desc, int value)
*/
void gpiod_set_raw_value(struct gpio_desc *desc, int value)
{
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return;
/* Should be using gpio_set_value_cansleep() */
WARN_ON(desc->chip->can_sleep);
@@ -2089,7 +2089,7 @@ EXPORT_SYMBOL_GPL(gpiod_set_raw_value);
*/
void gpiod_set_value(struct gpio_desc *desc, int value)
{
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return;
/* Should be using gpio_set_value_cansleep() */
WARN_ON(desc->chip->can_sleep);
@@ -2106,7 +2106,7 @@ EXPORT_SYMBOL_GPL(gpiod_set_value);
*/
int gpiod_cansleep(const struct gpio_desc *desc)
{
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return 0;
return desc->chip->can_sleep;
}
@@ -2124,7 +2124,7 @@ int gpiod_to_irq(const struct gpio_desc *desc)
struct gpio_chip *chip;
int offset;
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return -EINVAL;
chip = desc->chip;
offset = gpio_chip_hwgpio(desc);
@@ -2144,7 +2144,7 @@ EXPORT_SYMBOL_GPL(gpiod_to_irq);
*/
int gpiod_lock_as_irq(struct gpio_desc *desc)
{
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return -EINVAL;
if (test_bit(FLAG_IS_OUT, &desc->flags)) {
@@ -2199,7 +2199,7 @@ EXPORT_SYMBOL_GPL(gpio_unlock_as_irq);
int gpiod_get_raw_value_cansleep(const struct gpio_desc *desc)
{
might_sleep_if(extra_checks);
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return 0;
return _gpiod_get_raw_value(desc);
}
@@ -2219,7 +2219,7 @@ int gpiod_get_value_cansleep(const struct gpio_desc *desc)
int value;
might_sleep_if(extra_checks);
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return 0;
value = _gpiod_get_raw_value(desc);
@@ -2243,7 +2243,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_value_cansleep);
void gpiod_set_raw_value_cansleep(struct gpio_desc *desc, int value)
{
might_sleep_if(extra_checks);
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return;
_gpiod_set_raw_value(desc, value);
}
@@ -2262,7 +2262,7 @@ EXPORT_SYMBOL_GPL(gpiod_set_raw_value_cansleep);
void gpiod_set_value_cansleep(struct gpio_desc *desc, int value)
{
might_sleep_if(extra_checks);
- if (!desc)
+ if (IS_ERR_OR_NULL(desc))
return;
if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
--
1.9.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] gpiolib: check if gpio_desc pointer is error or NULL
2014-03-18 10:41 [PATCH] gpiolib: check if gpio_desc pointer is error or NULL Ben Dooks
@ 2014-03-19 1:48 ` Alexandre Courbot
2014-03-19 8:33 ` Ben Dooks
0 siblings, 1 reply; 4+ messages in thread
From: Alexandre Courbot @ 2014-03-19 1:48 UTC (permalink / raw)
To: Ben Dooks
Cc: linux-kernel, Linus Walleij, linux-gpio@vger.kernel.org,
Linux Kernel Mailing List
On Tue, Mar 18, 2014 at 7:41 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> Some of the gpiod_ calls take a pointer to a gpio_desc as their
> argument but only check to see if it is NULL to validate the
> input.
>
> Calls such as devm_gpiod_get() return an error-pointer if they
> fail, so doing the following will not work:
>
> gpio = devm_gpiod_get(...);
> gpiod_direction_output(gpio, 0);
>
> The sequence produces an OOPS like:
>
> Unable to handle kernel paging request at virtual address fffffffe
>
> Change all calls that check for !desc to use IS_ERR_OR_NULL() to
> avoid these issues.
This change is certainly correct from a semantics point of view. Maybe
I'd argue that the burden is on the caller to check that gpiod_get()
returns a valid GPIO descriptor, but having extra security doesn't
hurt. However my problem with this change in its current form is that
it will hide such forgetfulnesses by making functions like
gpiod_get_value() fail silently instead of triggering a oops.
Could you make sure that any call of a gpiolib function on a non-valid
descriptor raises at least a message in the console, and while you are
at it harmonize the way these messages are output? Right now we are
using pr_debug(), pr_warn() or WARN_ON() depending on the location. I
would say that using an invalid GPIO descriptor is offending enough
that it should trigger a stack dump at every attempt.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gpiolib: check if gpio_desc pointer is error or NULL
2014-03-19 1:48 ` Alexandre Courbot
@ 2014-03-19 8:33 ` Ben Dooks
2014-03-19 8:37 ` Alexandre Courbot
0 siblings, 1 reply; 4+ messages in thread
From: Ben Dooks @ 2014-03-19 8:33 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-kernel, Linus Walleij, linux-gpio@vger.kernel.org,
Linux Kernel Mailing List
On 19/03/14 02:48, Alexandre Courbot wrote:
> On Tue, Mar 18, 2014 at 7:41 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
>> Some of the gpiod_ calls take a pointer to a gpio_desc as their
>> argument but only check to see if it is NULL to validate the
>> input.
>>
>> Calls such as devm_gpiod_get() return an error-pointer if they
>> fail, so doing the following will not work:
>>
>> gpio = devm_gpiod_get(...);
>> gpiod_direction_output(gpio, 0);
>>
>> The sequence produces an OOPS like:
>>
>> Unable to handle kernel paging request at virtual address fffffffe
>>
>> Change all calls that check for !desc to use IS_ERR_OR_NULL() to
>> avoid these issues.
>
> This change is certainly correct from a semantics point of view. Maybe
> I'd argue that the burden is on the caller to check that gpiod_get()
> returns a valid GPIO descriptor, but having extra security doesn't
> hurt. However my problem with this change in its current form is that
> it will hide such forgetfulnesses by making functions like
> gpiod_get_value() fail silently instead of triggering a oops.
On the other hand, it means that we do not have to keep checking
the validity of the pointer in the caller.
> Could you make sure that any call of a gpiolib function on a non-valid
> descriptor raises at least a message in the console, and while you are
> at it harmonize the way these messages are output? Right now we are
> using pr_debug(), pr_warn() or WARN_ON() depending on the location. I
> would say that using an invalid GPIO descriptor is offending enough
> that it should trigger a stack dump at every attempt.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] gpiolib: check if gpio_desc pointer is error or NULL
2014-03-19 8:33 ` Ben Dooks
@ 2014-03-19 8:37 ` Alexandre Courbot
0 siblings, 0 replies; 4+ messages in thread
From: Alexandre Courbot @ 2014-03-19 8:37 UTC (permalink / raw)
To: Ben Dooks
Cc: linux-kernel, Linus Walleij, linux-gpio@vger.kernel.org,
Linux Kernel Mailing List
On Wed, Mar 19, 2014 at 5:33 PM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> On 19/03/14 02:48, Alexandre Courbot wrote:
>>
>> On Tue, Mar 18, 2014 at 7:41 PM, Ben Dooks <ben.dooks@codethink.co.uk>
>> wrote:
>>>
>>> Some of the gpiod_ calls take a pointer to a gpio_desc as their
>>> argument but only check to see if it is NULL to validate the
>>> input.
>>>
>>> Calls such as devm_gpiod_get() return an error-pointer if they
>>> fail, so doing the following will not work:
>>>
>>> gpio = devm_gpiod_get(...);
>>> gpiod_direction_output(gpio, 0);
>>>
>>> The sequence produces an OOPS like:
>>>
>>> Unable to handle kernel paging request at virtual address
>>> fffffffe
>>>
>>> Change all calls that check for !desc to use IS_ERR_OR_NULL() to
>>> avoid these issues.
>>
>>
>> This change is certainly correct from a semantics point of view. Maybe
>> I'd argue that the burden is on the caller to check that gpiod_get()
>> returns a valid GPIO descriptor, but having extra security doesn't
>> hurt. However my problem with this change in its current form is that
>> it will hide such forgetfulnesses by making functions like
>> gpiod_get_value() fail silently instead of triggering a oops.
>
>
> On the other hand, it means that we do not have to keep checking
> the validity of the pointer in the caller.
A very scary perspective which I don't think we should support.
Especially since the pointer only needs to be checked once, after
gpiod_get() is called.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-03-19 8:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-18 10:41 [PATCH] gpiolib: check if gpio_desc pointer is error or NULL Ben Dooks
2014-03-19 1:48 ` Alexandre Courbot
2014-03-19 8:33 ` Ben Dooks
2014-03-19 8:37 ` Alexandre Courbot
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 NNTP newsgroup(s).