| Message ID | 20240124103929.66545-1-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | [libgpiod] core: ignore positive values returned by the GPIO_V2_GET_LINE ioctl() | expand |
On Wed, Jan 24, 2024 at 11:39:29AM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > If the kernel GPIO driver (erroneously) returns a positive value from one > of its callbacks, it may end up being propagated to user space as > a positive value returned by the call to ioctl(). Let's treat all > non-zero values as errors as GPIO uAPI ioctl()s are not expected to ever > return positive values. This should be addressed in the kernel but will > remain a problem on older or unpatched versions so we need to sanitize it > in user-space too. > > Reported-by: José Guilherme de Castro Rodrigues <joseguilhermebh@hotmail.com> > Fixes: b7ba732e6a93 ("treewide: libgpiod v2 implementation") > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > lib/chip.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/chip.c b/lib/chip.c > index 7c05e53..7bf40c6 100644 > --- a/lib/chip.c > +++ b/lib/chip.c > @@ -239,7 +239,7 @@ gpiod_chip_request_lines(struct gpiod_chip *chip, > return NULL; > > ret = ioctl(chip->fd, GPIO_V2_GET_LINE_IOCTL, &uapi_req); > - if (ret < 0) > + if (ret) > return NULL; > What is errno going to be here? Is errno set if the ioctl returns positive? Cheers, Kent.
On Wed, Jan 24, 2024 at 11:54 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Jan 24, 2024 at 11:39:29AM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > If the kernel GPIO driver (erroneously) returns a positive value from one > > of its callbacks, it may end up being propagated to user space as > > a positive value returned by the call to ioctl(). Let's treat all > > non-zero values as errors as GPIO uAPI ioctl()s are not expected to ever > > return positive values. This should be addressed in the kernel but will > > remain a problem on older or unpatched versions so we need to sanitize it > > in user-space too. > > > > Reported-by: José Guilherme de Castro Rodrigues <joseguilhermebh@hotmail.com> > > Fixes: b7ba732e6a93 ("treewide: libgpiod v2 implementation") > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > lib/chip.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/chip.c b/lib/chip.c > > index 7c05e53..7bf40c6 100644 > > --- a/lib/chip.c > > +++ b/lib/chip.c > > @@ -239,7 +239,7 @@ gpiod_chip_request_lines(struct gpiod_chip *chip, > > return NULL; > > > > ret = ioctl(chip->fd, GPIO_V2_GET_LINE_IOCTL, &uapi_req); > > - if (ret < 0) > > + if (ret) > > return NULL; > > > > What is errno going to be here? > Is errno set if the ioctl returns positive? > No it isn't, thanks for catching it. This patch is incomplete - we need a wrapper around ioctl() for all uAPI calls that will check for positive numbers and set errno to ERANGE (is that the right one? any other ideas?) then return -1. Bart
On Wed, Jan 24, 2024 at 11:58:16AM +0100, Bartosz Golaszewski wrote: > On Wed, Jan 24, 2024 at 11:54 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Wed, Jan 24, 2024 at 11:39:29AM +0100, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > If the kernel GPIO driver (erroneously) returns a positive value from one > > > of its callbacks, it may end up being propagated to user space as > > > a positive value returned by the call to ioctl(). Let's treat all > > > non-zero values as errors as GPIO uAPI ioctl()s are not expected to ever > > > return positive values. This should be addressed in the kernel but will > > > remain a problem on older or unpatched versions so we need to sanitize it > > > in user-space too. > > > > > > Reported-by: José Guilherme de Castro Rodrigues <joseguilhermebh@hotmail.com> > > > Fixes: b7ba732e6a93 ("treewide: libgpiod v2 implementation") > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > --- > > > lib/chip.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/chip.c b/lib/chip.c > > > index 7c05e53..7bf40c6 100644 > > > --- a/lib/chip.c > > > +++ b/lib/chip.c > > > @@ -239,7 +239,7 @@ gpiod_chip_request_lines(struct gpiod_chip *chip, > > > return NULL; > > > > > > ret = ioctl(chip->fd, GPIO_V2_GET_LINE_IOCTL, &uapi_req); > > > - if (ret < 0) > > > + if (ret) > > > return NULL; > > > > > > > What is errno going to be here? > > Is errno set if the ioctl returns positive? > > > > No it isn't, thanks for catching it. > > This patch is incomplete - we need a wrapper around ioctl() for all > uAPI calls that will check for positive numbers and set errno to > ERANGE (is that the right one? any other ideas?) then return -1. > The two things I'm looking for in an error code would be that we don't use it already, and it isn't too confusing. ERANGE is typically for numeric overlow, so a bit confusing. EBADE? Though EHWPOISON does seem fitting given the root cause ;-). Cheers, Kent.
On Wed, Jan 24, 2024 at 12:29 PM Kent Gibson <warthog618@gmail.com> wrote: > > On Wed, Jan 24, 2024 at 11:58:16AM +0100, Bartosz Golaszewski wrote: > > On Wed, Jan 24, 2024 at 11:54 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > On Wed, Jan 24, 2024 at 11:39:29AM +0100, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > If the kernel GPIO driver (erroneously) returns a positive value from one > > > > of its callbacks, it may end up being propagated to user space as > > > > a positive value returned by the call to ioctl(). Let's treat all > > > > non-zero values as errors as GPIO uAPI ioctl()s are not expected to ever > > > > return positive values. This should be addressed in the kernel but will > > > > remain a problem on older or unpatched versions so we need to sanitize it > > > > in user-space too. > > > > > > > > Reported-by: José Guilherme de Castro Rodrigues <joseguilhermebh@hotmail.com> > > > > Fixes: b7ba732e6a93 ("treewide: libgpiod v2 implementation") > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > --- > > > > lib/chip.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/lib/chip.c b/lib/chip.c > > > > index 7c05e53..7bf40c6 100644 > > > > --- a/lib/chip.c > > > > +++ b/lib/chip.c > > > > @@ -239,7 +239,7 @@ gpiod_chip_request_lines(struct gpiod_chip *chip, > > > > return NULL; > > > > > > > > ret = ioctl(chip->fd, GPIO_V2_GET_LINE_IOCTL, &uapi_req); > > > > - if (ret < 0) > > > > + if (ret) > > > > return NULL; > > > > > > > > > > What is errno going to be here? > > > Is errno set if the ioctl returns positive? > > > > > > > No it isn't, thanks for catching it. > > > > This patch is incomplete - we need a wrapper around ioctl() for all > > uAPI calls that will check for positive numbers and set errno to > > ERANGE (is that the right one? any other ideas?) then return -1. > > > > The two things I'm looking for in an error code would be that we don't > use it already, and it isn't too confusing. > > ERANGE is typically for numeric overlow, so a bit confusing. > > EBADE? Yeah, I think this one will do. I was thinking about EREMOTEIO but "Invalid exchange" sounds the most fitting. Bart > > Though EHWPOISON does seem fitting given the root cause ;-). > > Cheers, > Kent. > >
diff --git a/lib/chip.c b/lib/chip.c index 7c05e53..7bf40c6 100644 --- a/lib/chip.c +++ b/lib/chip.c @@ -239,7 +239,7 @@ gpiod_chip_request_lines(struct gpiod_chip *chip, return NULL; ret = ioctl(chip->fd, GPIO_V2_GET_LINE_IOCTL, &uapi_req); - if (ret < 0) + if (ret) return NULL; request = gpiod_line_request_from_uapi(&uapi_req, info.name);