Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1652962 > unrolled thread

[PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization Bartosz Golaszewski <brgl@bgdev.pl> - 2017-05-30 11:00 +0200
    Re: [PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-30 21:00 +0200
      Re: [PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization Bartosz Golaszewski <brgl@bgdev.pl> - 2017-05-31 13:00 +0200
        Re: [PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-31 20:10 +0200

#1652962 — [PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization

We're currently only checking the first character of the input to the
debugfs event files, so a string like '0sdfdsf' is valid and indicates
a falling edge event.

Be more strict and only allow '0', '1', '0\n' & '1\n'.

While we're at it: move the sanitization code before the irq_enabled
check so that we indicate an error on invalid input even if nobody is
waiting for events.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index ba8d62a..da76267 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -208,8 +208,7 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	struct seq_file *sfile;
 	struct gpio_desc *desc;
 	struct gpio_chip *gc;
-	int val;
-	char buf;
+	int rv, val;
 
 	sfile = file->private_data;
 	priv = sfile->private;
@@ -217,19 +216,15 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	chip = priv->chip;
 	gc = &chip->gc;
 
+	rv = kstrtoint_from_user(usr_buf, size, 0, &val);
+	if (rv)
+		return rv;
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
 	if (!chip->lines[priv->offset].irq_enabled)
 		return size;
 
-	if (copy_from_user(&buf, usr_buf, 1))
-		return -EFAULT;
-
-	if (buf == '0')
-		val = 0;
-	else if (buf == '1')
-		val = 1;
-	else
-		return -EINVAL;
-
 	gpiod_set_value_cansleep(desc, val);
 	priv->chip->irq_ctx.irq = gc->irq_base + priv->offset;
 	irq_work_queue(&priv->chip->irq_ctx.work);
-- 
2.9.3

[toc] | [next] | [standalone]

#1653498

On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> We're currently only checking the first character of the input to the
> debugfs event files, so a string like '0sdfdsf' is valid and indicates
> a falling edge event.
>
> Be more strict and only allow '0', '1', '0\n' & '1\n'.
>
> While we're at it: move the sanitization code before the irq_enabled
> check so that we indicate an error on invalid input even if nobody is
> waiting for events.

> -       int val;
> -       char buf;
> +       int rv, val;

> +       rv = kstrtoint_from_user(usr_buf, size, 0, &val);
> +       if (rv)
> +               return rv;

> +       if (val != 0 && val != 1)

Wouldn't be easier to have

u8 rv;

ret = kstrtu8_from_user();
if (ret >= 2)
 return ...;

?

> +               return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1654123

2017-05-30 20:52 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> We're currently only checking the first character of the input to the
>> debugfs event files, so a string like '0sdfdsf' is valid and indicates
>> a falling edge event.
>>
>> Be more strict and only allow '0', '1', '0\n' & '1\n'.
>>
>> While we're at it: move the sanitization code before the irq_enabled
>> check so that we indicate an error on invalid input even if nobody is
>> waiting for events.
>
>> -       int val;
>> -       char buf;
>> +       int rv, val;
>
>> +       rv = kstrtoint_from_user(usr_buf, size, 0, &val);
>> +       if (rv)
>> +               return rv;
>
>> +       if (val != 0 && val != 1)
>
> Wouldn't be easier to have
>
> u8 rv;
>
> ret = kstrtu8_from_user();
> if (ret >= 2)
>  return ...;
>
> ?

kstrtu8_from_user() doesn't return the converted value, so you won't
skip an if anyway and by using the int variant, we're avoiding a cast.
I'd prefer it this way frankly.

Thanks,
Bartosz

[toc] | [prev] | [next] | [standalone]

#1654513

On Wed, May 31, 2017 at 1:52 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2017-05-30 20:52 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

>>> +       if (val != 0 && val != 1)
>>
>> Wouldn't be easier to have
>>
>> u8 rv;
>>
>> ret = kstrtu8_from_user();
>> if (ret >= 2)
>>  return ...;
>>
>> ?
>
> kstrtu8_from_user() doesn't return the converted value, so you won't
> skip an if anyway

Oh, yes.

> and by using the int variant, we're avoiding a cast.
> I'd prefer it this way frankly.

Fair enough. (Though I would go with (val < 0 && val > 1) condition,
of course it's matter of taste)

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web