[v1] core: Relax gpiod_chip_open() for symbolic links
diff mbox series

Message ID 20200206181358.12805-1-andriy.shevchenko@linux.intel.com
State New
Headers show
Series
  • [v1] core: Relax gpiod_chip_open() for symbolic links
Related show

Commit Message

Andy Shevchenko Feb. 6, 2020, 6:13 p.m. UTC
User may ask device helper tool, for example, udev, to create a specific
symbolic link to a device node. GPIO chip character device node is not
exceptional. However, libgpiod in the commit d9b1c1f14c6b
("core: harden gpiod_chip_open()") went way too far in the hardening device
node check.

Relax that hardening for symbolic link to fix the regression.

Reproducer:

  % gpioinfo /dev/gpiochip5
  gpiochip5 - 16 lines:
      line   0:  "MUX33_DIR" "uart1-rx-oe" output active-high [used]
      ...

  % ln -sf /dev/gpiochip5 /dev/MyGPIO_5

  % gpioinfo /dev/MyGPIO_5
  gpioinfo: looking up chip /dev/MyGPIO_5: Inappropriate ioctl for device

Link: https://stackoverflow.com/questions/60057494/gpio-issue-with-sym-link
Fixes: d9b1c1f14c6b ("core: harden gpiod_chip_open()")
Cc: Bartosz Golaszewski <bartekgola@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 lib/core.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

Comments

Bartosz Golaszewski Feb. 7, 2020, 10:13 a.m. UTC | #1
czw., 6 lut 2020 o 19:14 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> User may ask device helper tool, for example, udev, to create a specific
> symbolic link to a device node. GPIO chip character device node is not
> exceptional. However, libgpiod in the commit d9b1c1f14c6b
> ("core: harden gpiod_chip_open()") went way too far in the hardening device
> node check.
>
> Relax that hardening for symbolic link to fix the regression.
>
> Reproducer:
>
>   % gpioinfo /dev/gpiochip5
>   gpiochip5 - 16 lines:
>       line   0:  "MUX33_DIR" "uart1-rx-oe" output active-high [used]
>       ...
>
>   % ln -sf /dev/gpiochip5 /dev/MyGPIO_5
>
>   % gpioinfo /dev/MyGPIO_5
>   gpioinfo: looking up chip /dev/MyGPIO_5: Inappropriate ioctl for device
>
> Link: https://stackoverflow.com/questions/60057494/gpio-issue-with-sym-link
> Fixes: d9b1c1f14c6b ("core: harden gpiod_chip_open()")
> Cc: Bartosz Golaszewski <bartekgola@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Hi Andy,

thanks for this - it makes perfect sense. One nit though: could you
keep the includes ordered alphabetically? Also: it would be great if
you could add a test case for this to tests/tests-chip.c.

Bartosz
Andy Shevchenko Feb. 7, 2020, 10:30 a.m. UTC | #2
On Fri, Feb 07, 2020 at 11:13:43AM +0100, Bartosz Golaszewski wrote:
> czw., 6 lut 2020 o 19:14 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> >
> > User may ask device helper tool, for example, udev, to create a specific
> > symbolic link to a device node. GPIO chip character device node is not
> > exceptional. However, libgpiod in the commit d9b1c1f14c6b
> > ("core: harden gpiod_chip_open()") went way too far in the hardening device
> > node check.
> >
> > Relax that hardening for symbolic link to fix the regression.
> >
> > Reproducer:
> >
> >   % gpioinfo /dev/gpiochip5
> >   gpiochip5 - 16 lines:
> >       line   0:  "MUX33_DIR" "uart1-rx-oe" output active-high [used]
> >       ...
> >
> >   % ln -sf /dev/gpiochip5 /dev/MyGPIO_5
> >
> >   % gpioinfo /dev/MyGPIO_5
> >   gpioinfo: looking up chip /dev/MyGPIO_5: Inappropriate ioctl for device
> >
> > Link: https://stackoverflow.com/questions/60057494/gpio-issue-with-sym-link
> > Fixes: d9b1c1f14c6b ("core: harden gpiod_chip_open()")
> > Cc: Bartosz Golaszewski <bartekgola@gmail.com>
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Hi Andy,
> 
> thanks for this - it makes perfect sense. One nit though: could you
> keep the includes ordered alphabetically?

Probably not. The user space relies a lot on header ordering. And limits.h
sounds like one needed to be included first in many cases. That's why I moved
it to the top. I can do it if you insist, but I consider it wrong approach for
the record.

> Also: it would be great if
> you could add a test case for this to tests/tests-chip.c.

I will look at it if I can do quickly something.

For the record I have tested it on Intel Edison with real GPIO chips (so,
that's how reproducer code appears in the commit message).

Thanks for review!
Bartosz Golaszewski Feb. 7, 2020, 11:01 a.m. UTC | #3
pt., 7 lut 2020 o 11:30 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Fri, Feb 07, 2020 at 11:13:43AM +0100, Bartosz Golaszewski wrote:
> > czw., 6 lut 2020 o 19:14 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> napisał(a):
> > >
> > > User may ask device helper tool, for example, udev, to create a specific
> > > symbolic link to a device node. GPIO chip character device node is not
> > > exceptional. However, libgpiod in the commit d9b1c1f14c6b
> > > ("core: harden gpiod_chip_open()") went way too far in the hardening device
> > > node check.
> > >
> > > Relax that hardening for symbolic link to fix the regression.
> > >
> > > Reproducer:
> > >
> > >   % gpioinfo /dev/gpiochip5
> > >   gpiochip5 - 16 lines:
> > >       line   0:  "MUX33_DIR" "uart1-rx-oe" output active-high [used]
> > >       ...
> > >
> > >   % ln -sf /dev/gpiochip5 /dev/MyGPIO_5
> > >
> > >   % gpioinfo /dev/MyGPIO_5
> > >   gpioinfo: looking up chip /dev/MyGPIO_5: Inappropriate ioctl for device
> > >
> > > Link: https://stackoverflow.com/questions/60057494/gpio-issue-with-sym-link
> > > Fixes: d9b1c1f14c6b ("core: harden gpiod_chip_open()")
> > > Cc: Bartosz Golaszewski <bartekgola@gmail.com>
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > Hi Andy,
> >
> > thanks for this - it makes perfect sense. One nit though: could you
> > keep the includes ordered alphabetically?
>
> Probably not. The user space relies a lot on header ordering. And limits.h
> sounds like one needed to be included first in many cases. That's why I moved
> it to the top. I can do it if you insist, but I consider it wrong approach for
> the record.

Nah, if anything headers may rely on some preprocessor defines coming
before them, but the ordering should be of no importance.

>
> > Also: it would be great if
> > you could add a test case for this to tests/tests-chip.c.
>
> I will look at it if I can do quickly something.
>

If not, don't worry - I can add it later myself.

Bartosz

> For the record I have tested it on Intel Edison with real GPIO chips (so,
> that's how reproducer code appears in the commit message).
>
> Thanks for review!
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Andy Shevchenko Feb. 7, 2020, 11:28 a.m. UTC | #4
On Fri, Feb 07, 2020 at 12:01:46PM +0100, Bartosz Golaszewski wrote:
> pt., 7 lut 2020 o 11:30 Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> napisał(a):
> > On Fri, Feb 07, 2020 at 11:13:43AM +0100, Bartosz Golaszewski wrote:
> > > czw., 6 lut 2020 o 19:14 Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> napisał(a):

...

> > > thanks for this - it makes perfect sense. One nit though: could you
> > > keep the includes ordered alphabetically?
> >
> > Probably not. The user space relies a lot on header ordering. And limits.h
> > sounds like one needed to be included first in many cases. That's why I moved
> > it to the top. I can do it if you insist, but I consider it wrong approach for
> > the record.
> 
> Nah, if anything headers may rely on some preprocessor defines coming
> before them, but the ordering should be of no importance.

Okay, in any case, if you think it's better to be sorted, can you change it
when applying? (I don't think we need another version simple for that)

> > > Also: it would be great if
> > > you could add a test case for this to tests/tests-chip.c.
> >
> > I will look at it if I can do quickly something.

> If not, don't worry - I can add it later myself.

I briefly looked at this, but it seems not feasible to me in reasonable time,
sorry.

The problems I encountered are, but not limited to:
- creating a symlink in a test case folder
- understanding how to handle interrupt of the test case (we have to remove
  link ourselves or framework does it for us?)
- where to put the symbolic link creation: I think it might be a (boolean)
  parameter to gpio-mockup testing API when we "make a chip" (when device node
  should appear) to enable symlink with a predefined name (like
  $testpath/gpiochipX-link)
- last time I did something (simple!) with GLib was several years ago
Bartosz Gołaszewski Feb. 7, 2020, 12:05 p.m. UTC | #5
pt., 7 lut 2020 o 12:28 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> On Fri, Feb 07, 2020 at 12:01:46PM +0100, Bartosz Golaszewski wrote:
> > pt., 7 lut 2020 o 11:30 Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> napisał(a):
> > > On Fri, Feb 07, 2020 at 11:13:43AM +0100, Bartosz Golaszewski wrote:
> > > > czw., 6 lut 2020 o 19:14 Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> napisał(a):
>
> ...
>
> > > > thanks for this - it makes perfect sense. One nit though: could you
> > > > keep the includes ordered alphabetically?
> > >
> > > Probably not. The user space relies a lot on header ordering. And limits.h
> > > sounds like one needed to be included first in many cases. That's why I moved
> > > it to the top. I can do it if you insist, but I consider it wrong approach for
> > > the record.
> >
> > Nah, if anything headers may rely on some preprocessor defines coming
> > before them, but the ordering should be of no importance.
>
> Okay, in any case, if you think it's better to be sorted, can you change it
> when applying? (I don't think we need another version simple for that)
>

Sure.

> > > > Also: it would be great if
> > > > you could add a test case for this to tests/tests-chip.c.
> > >
> > > I will look at it if I can do quickly something.
>
> > If not, don't worry - I can add it later myself.
>
> I briefly looked at this, but it seems not feasible to me in reasonable time,
> sorry.
>
> The problems I encountered are, but not limited to:
> - creating a symlink in a test case folder
> - understanding how to handle interrupt of the test case (we have to remove
>   link ourselves or framework does it for us?)
> - where to put the symbolic link creation: I think it might be a (boolean)
>   parameter to gpio-mockup testing API when we "make a chip" (when device node
>   should appear) to enable symlink with a predefined name (like
>   $testpath/gpiochipX-link)
> - last time I did something (simple!) with GLib was several years ago
>

Sure, I can do it myself - it will probably take me much less time.

Bart
Bartosz Golaszewski Feb. 7, 2020, 1 p.m. UTC | #6
czw., 6 lut 2020 o 19:14 Andy Shevchenko
<andriy.shevchenko@linux.intel.com> napisał(a):
>
> User may ask device helper tool, for example, udev, to create a specific
> symbolic link to a device node. GPIO chip character device node is not
> exceptional. However, libgpiod in the commit d9b1c1f14c6b
> ("core: harden gpiod_chip_open()") went way too far in the hardening device
> node check.
>
> Relax that hardening for symbolic link to fix the regression.
>
> Reproducer:
>
>   % gpioinfo /dev/gpiochip5
>   gpiochip5 - 16 lines:
>       line   0:  "MUX33_DIR" "uart1-rx-oe" output active-high [used]
>       ...
>
>   % ln -sf /dev/gpiochip5 /dev/MyGPIO_5
>
>   % gpioinfo /dev/MyGPIO_5
>   gpioinfo: looking up chip /dev/MyGPIO_5: Inappropriate ioctl for device
>
> Link: https://stackoverflow.com/questions/60057494/gpio-issue-with-sym-link
> Fixes: d9b1c1f14c6b ("core: harden gpiod_chip_open()")
> Cc: Bartosz Golaszewski <bartekgola@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Patch applied and backported to v1.4.x and v1.5.x stable branches.

Thanks!
Bartosz

Patch
diff mbox series

diff --git a/lib/core.c b/lib/core.c
index 8352e18..32476ee 100644
--- a/lib/core.c
+++ b/lib/core.c
@@ -8,6 +8,7 @@ 
 /* Low-level, core library code. */
 
 #include <errno.h>
+#include <limits.h>
 #include <fcntl.h>
 #include <gpiod.h>
 #include <linux/gpio.h>
@@ -75,7 +76,7 @@  struct gpiod_chip {
 
 static bool is_gpiochip_cdev(const char *path)
 {
-	char *name, *pathcpy, *sysfsp, sysfsdev[16], devstr[16];
+	char *name, *realname, *sysfsp, sysfsdev[16], devstr[16];
 	struct stat statbuf;
 	bool ret = false;
 	int rv, fd;
@@ -85,6 +86,21 @@  static bool is_gpiochip_cdev(const char *path)
 	if (rv)
 		goto out;
 
+	/*
+	 * Is it a symbolic link?
+	 * We have to resolve symbolic link before checking the rest.
+	 */
+	if (S_ISLNK(statbuf.st_mode))
+		realname = realpath(path, NULL);
+	else
+		realname = strdup(path);
+	if (realname == NULL)
+		goto out;
+
+	rv = stat(realname, &statbuf);
+	if (rv)
+		goto out_free_realname;
+
 	/* Is it a character device? */
 	if (!S_ISCHR(statbuf.st_mode)) {
 		/*
@@ -94,20 +110,16 @@  static bool is_gpiochip_cdev(const char *path)
 		 * libgpiod from before the introduction of this routine.
 		 */
 		errno = ENOTTY;
-		goto out;
+		goto out_free_realname;
 	}
 
 	/* Get the basename. */
-	pathcpy = strdup(path);
-	if (!pathcpy)
-		goto out;
-
-	name = basename(pathcpy);
+	name = basename(realname);
 
 	/* Do we have a corresponding sysfs attribute? */
 	rv = asprintf(&sysfsp, "/sys/bus/gpio/devices/%s/dev", name);
 	if (rv < 0)
-		goto out_free_pathcpy;
+		goto out_free_realname;
 
 	if (access(sysfsp, R_OK) != 0) {
 		/*
@@ -149,8 +161,8 @@  static bool is_gpiochip_cdev(const char *path)
 
 out_free_sysfsp:
 	free(sysfsp);
-out_free_pathcpy:
-	free(pathcpy);
+out_free_realname:
+	free(realname);
 out:
 	return ret;
 }