| Message ID | 150131523385.6834.16274817727659878191.stgit@devbox |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 29, 2017 at 11:00 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > Check user-given gpio number and reject it before > calling gpio_to_desc() because gpio_to_desc() is > for kernel driver and it expects given gpio number > is valid (means 0 to 511). > If given number is invalid, gpio_to_desc() calls > WARN() and dump registers and stack for debug. > This means user can easily kick WARN() just by > writing invalid gpio number (e.g. 512) to > /sys/class/gpio/export. > This bug has been introduced by commit 0e9a5edf5d01 > ("gpio: fix deferred probe detection for legacy API") Why so narrow paraghraps? We don't do two column articles :-) > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Fixes: commit 0e9a5edf5d01 ("gpio: fix deferred probe detection for legacy API") If you use git tools directly, like % git commit -s --amend you will immediately notice that your SoB line is supposed to be the last one. Besides above, word "commit" is not needed in Fixes: tag IIRC. > --- a/drivers/gpio/gpiolib-sysfs.c > +++ b/drivers/gpio/gpiolib-sysfs.c > @@ -2,6 +2,7 @@ > #include <linux/mutex.h> > #include <linux/device.h> > #include <linux/sysfs.h> > +#include <linux/gpio.h> > #include <linux/gpio/consumer.h> > #include <linux/gpio/driver.h> > #include <linux/interrupt.h> > @@ -443,14 +444,16 @@ static ssize_t export_store(struct class *class, > - struct gpio_desc *desc; > + struct gpio_desc *desc = NULL; > - desc = gpio_to_desc(gpio); > + if (gpio_is_valid(gpio)) > + desc = gpio_to_desc(gpio); Twice repeatition is a candidate for a helper, like: static inline struct gpio_desc *gpio_to_valid_desc(int gpio) { return gpio_is_valid(gpio) ? gpio_to_desc(gpio) : NULL; }
On Sun, 30 Jul 2017 19:21:36 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Sat, Jul 29, 2017 at 11:00 AM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > Check user-given gpio number and reject it before > > calling gpio_to_desc() because gpio_to_desc() is > > for kernel driver and it expects given gpio number > > is valid (means 0 to 511). > > > If given number is invalid, gpio_to_desc() calls > > WARN() and dump registers and stack for debug. > > This means user can easily kick WARN() just by > > writing invalid gpio number (e.g. 512) to > > /sys/class/gpio/export. > > > This bug has been introduced by commit 0e9a5edf5d01 > > ("gpio: fix deferred probe detection for legacy API") > > Why so narrow paraghraps? We don't do two column articles :-) OK, I'll remove this, since Fixes explain that. > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > Fixes: commit 0e9a5edf5d01 ("gpio: fix deferred probe detection for legacy API") > > If you use git tools directly, like > > % git commit -s --amend > > you will immediately notice that your SoB line is supposed to be the last one. I'll move SoB line to the bottom. > > Besides above, word "commit" is not needed in Fixes: tag IIRC. Ah, OK, I'll remove "commit" from Fixes. > > > > --- a/drivers/gpio/gpiolib-sysfs.c > > +++ b/drivers/gpio/gpiolib-sysfs.c > > @@ -2,6 +2,7 @@ > > #include <linux/mutex.h> > > #include <linux/device.h> > > #include <linux/sysfs.h> > > +#include <linux/gpio.h> > > #include <linux/gpio/consumer.h> > > #include <linux/gpio/driver.h> > > #include <linux/interrupt.h> > > > @@ -443,14 +444,16 @@ static ssize_t export_store(struct class *class, > > > - struct gpio_desc *desc; > > + struct gpio_desc *desc = NULL; > > > - desc = gpio_to_desc(gpio); > > + if (gpio_is_valid(gpio)) > > + desc = gpio_to_desc(gpio); > > Twice repeatition is a candidate for a helper, like: > > static inline struct gpio_desc *gpio_to_valid_desc(int gpio) > { > return gpio_is_valid(gpio) ? gpio_to_desc(gpio) : NULL; > } OK, I'll add the helper. Thanks,
diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c index 4b44dd97c07f..4efb11fd1c22 100644 --- a/drivers/gpio/gpiolib-sysfs.c +++ b/drivers/gpio/gpiolib-sysfs.c @@ -2,6 +2,7 @@ #include <linux/mutex.h> #include <linux/device.h> #include <linux/sysfs.h> +#include <linux/gpio.h> #include <linux/gpio/consumer.h> #include <linux/gpio/driver.h> #include <linux/interrupt.h> @@ -443,14 +444,16 @@ static ssize_t export_store(struct class *class, const char *buf, size_t len) { long gpio; - struct gpio_desc *desc; + struct gpio_desc *desc = NULL; int status; status = kstrtol(buf, 0, &gpio); if (status < 0) goto done; - desc = gpio_to_desc(gpio); + if (gpio_is_valid(gpio)) + desc = gpio_to_desc(gpio); + /* reject invalid GPIOs */ if (!desc) { pr_warn("%s: invalid GPIO %ld\n", __func__, gpio); @@ -485,14 +488,16 @@ static ssize_t unexport_store(struct class *class, const char *buf, size_t len) { long gpio; - struct gpio_desc *desc; + struct gpio_desc *desc = NULL; int status; status = kstrtol(buf, 0, &gpio); if (status < 0) goto done; - desc = gpio_to_desc(gpio); + if (gpio_is_valid(gpio)) + desc = gpio_to_desc(gpio); + /* reject bogus commands (gpio_unexport ignores them) */ if (!desc) { pr_warn("%s: invalid GPIO %ld\n", __func__, gpio);
Check user-given gpio number and reject it before calling gpio_to_desc() because gpio_to_desc() is for kernel driver and it expects given gpio number is valid (means 0 to 511). If given number is invalid, gpio_to_desc() calls WARN() and dump registers and stack for debug. This means user can easily kick WARN() just by writing invalid gpio number (e.g. 512) to /sys/class/gpio/export. This bug has been introduced by commit 0e9a5edf5d01 ("gpio: fix deferred probe detection for legacy API") Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Fixes: commit 0e9a5edf5d01 ("gpio: fix deferred probe detection for legacy API") --- drivers/gpio/gpiolib-sysfs.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html