| Message ID | 20200206181358.12805-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] core: Relax gpiod_chip_open() for symbolic links | expand |
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
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!
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 > >
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
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
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
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; }
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(-)