| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Yx8Bj0+4STpklMc2@sol>
Date: Mon, 12 Sep 2022 17:53:19 +0800
From: Kent Gibson <warthog618@...il.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Viresh Kumar <viresh.kumar@...aro.org>,
linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] gpiolib: cdev: export the consumer's PID
On Mon, Sep 12, 2022 at 10:52:53AM +0200, Bartosz Golaszewski wrote:
> On Sat, Sep 10, 2022 at 4:52 PM Kent Gibson <warthog618@...il.com> wrote:
> >
> > On Fri, Sep 09, 2022 at 02:13:29PM +0200, Bartosz Golaszewski wrote:
> > > It's useful for user-space to be able to know the PIDs of processes
> > > holding GPIO lines in case they have the permissions and need to kill
> > > them.
> > >
> >
> > "the PID of the process holding a GPIO line"
> >
> > As written it could be interpreted to imply that multiple processes can
> > hold a line, so go singular to remove that possibility.
> >
> > > Extend the gpio_v2_line_info structure with the consumer_pid field
> > > that's set to the PID of the user-space process or 0 if the user lives
> > > in the kernel.
> > >
> > > Signed-off-by: Bartosz Golaszewski <brgl@...ev.pl>
> > > ---
> > > drivers/gpio/gpiolib-cdev.c | 2 ++
> > > drivers/gpio/gpiolib.c | 24 +++++++++++++++++++-----
> > > drivers/gpio/gpiolib.h | 2 ++
> > > include/uapi/linux/gpio.h | 5 ++++-
> > > 4 files changed, 27 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > > index f8041d4898d1..9b6d518680dc 100644
> > > --- a/drivers/gpio/gpiolib-cdev.c
> > > +++ b/drivers/gpio/gpiolib-cdev.c
> > > @@ -2120,6 +2120,8 @@ static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> > > if (desc->label)
> > > strscpy(info->consumer, desc->label, sizeof(info->consumer));
> > >
> > > + info->consumer_pid = desc->pid;
> > > +
> > > /*
> > > * Userspace only need to know that the kernel is using this GPIO so
> > > * it can't use it.
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index 6768734b9e15..0c9d1639b04d 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -96,6 +96,11 @@ static inline void desc_set_label(struct gpio_desc *d, const char *label)
> > > d->label = label;
> > > }
> > >
> > > +static inline void desc_set_pid(struct gpio_desc *d, pid_t pid)
> > > +{
> > > + d->pid = pid;
> > > +}
> > > +
> > > /**
> > > * gpio_to_desc - Convert a GPIO number to its descriptor
> > > * @gpio: global GPIO number
> > > @@ -1892,7 +1897,8 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
> > > * on each other, and help provide better diagnostics in debugfs.
> > > * They're called even less than the "set direction" calls.
> > > */
> > > -static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > > +static int
> > > +gpiod_request_commit(struct gpio_desc *desc, const char *label, pid_t pid)
> > > {
> > > struct gpio_chip *gc = desc->gdev->chip;
> > > int ret;
> > > @@ -1913,6 +1919,7 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
> > >
> > > if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
> > > desc_set_label(desc, label ? : "?");
> > > + desc_set_pid(desc, pid);
> > > } else {
> > > ret = -EBUSY;
> > > goto out_free_unlock;
> > > @@ -1987,7 +1994,8 @@ static int validate_desc(const struct gpio_desc *desc, const char *func)
> > > return; \
> > > } while (0)
> > >
> > > -int gpiod_request(struct gpio_desc *desc, const char *label)
> > > +static int
> > > +gpiod_request_with_pid(struct gpio_desc *desc, const char *label, pid_t pid)
> > > {
> > > int ret = -EPROBE_DEFER;
> > > struct gpio_device *gdev;
> > > @@ -1996,7 +2004,7 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
> > > gdev = desc->gdev;
> > >
> > > if (try_module_get(gdev->owner)) {
> > > - ret = gpiod_request_commit(desc, label);
> > > + ret = gpiod_request_commit(desc, label, pid);
> > > if (ret)
> > > module_put(gdev->owner);
> > > else
> > > @@ -2009,11 +2017,16 @@ int gpiod_request(struct gpio_desc *desc, const char *label)
> > > return ret;
> > > }
> > >
> > > +int gpiod_request(struct gpio_desc *desc, const char *label)
> > > +{
> > > + return gpiod_request_with_pid(desc, label, 0);
> > > +}
> > > +
> > > int gpiod_request_user(struct gpio_desc *desc, const char *label)
> > > {
> > > int ret;
> > >
> > > - ret = gpiod_request(desc, label);
> > > + ret = gpiod_request_with_pid(desc, label, current->pid);
> > > if (ret == -EPROBE_DEFER)
> > > ret = -ENODEV;
> > >
> > > @@ -2042,6 +2055,7 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
> > > }
> > > kfree_const(desc->label);
> > > desc_set_label(desc, NULL);
> > > + desc_set_pid(desc, 0);
> > > clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
> > > clear_bit(FLAG_REQUESTED, &desc->flags);
> > > clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
> > > @@ -2140,7 +2154,7 @@ struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
> > > return desc;
> > > }
> > >
> > > - ret = gpiod_request_commit(desc, label);
> > > + ret = gpiod_request_commit(desc, label, 0);
> > > if (ret < 0)
> > > return ERR_PTR(ret);
> > >
> > > diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
> > > index b35deb08a7f5..d1535677e162 100644
> > > --- a/drivers/gpio/gpiolib.h
> > > +++ b/drivers/gpio/gpiolib.h
> > > @@ -165,6 +165,8 @@ struct gpio_desc {
> > >
> > > /* Connection label */
> > > const char *label;
> > > + /* Consumer's PID (if consumer is in user-space, otherwise 0) */
> > > + pid_t pid;
> > > /* Name of the GPIO */
> > > const char *name;
> > > #ifdef CONFIG_OF_DYNAMIC
> > > diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> > > index cb9966d49a16..37f10021d1aa 100644
> > > --- a/include/uapi/linux/gpio.h
> > > +++ b/include/uapi/linux/gpio.h
> > > @@ -219,6 +219,8 @@ struct gpio_v2_line_request {
> > > * gpio_v2_line_flag, such as %GPIO_V2_LINE_FLAG_ACTIVE_LOW,
> > > * %GPIO_V2_LINE_FLAG_OUTPUT etc, added together.
> > > * @attrs: the configuration attributes associated with the line
> > > + * @consumer_pid: process ID of the user-space consumer or 0 if the consumer
> > > + * lives in kernel space
> > > * @padding: reserved for future use
> > > */
> > > struct gpio_v2_line_info {
> > > @@ -228,8 +230,9 @@ struct gpio_v2_line_info {
> > > __u32 num_attrs;
> > > __aligned_u64 flags;
> > > struct gpio_v2_line_attribute attrs[GPIO_V2_LINE_NUM_ATTRS_MAX];
> > > + __s32 consumer_pid;
> >
> > My knee-jerk reaction here was to make the pid unsigned, as we never
> > pass a negative PID.
> > Keeping in mind that the existing kernel will return 0 for this field
> > (the existing padding), so 0 needs to be excluded from valid PIDs
> > anyway.
> >
> > Andy suggests returning -1 for kernel held lines.
> > In that case 0 would mean "old kernel", while -1 would mean "kernel
> > held".
> >
> > As libgpiod will have to convert the 0 to -1 when returning the PID to
> > user-space as a pid_t, I'm good with the uAPI using 0 to mean
> > "no PID available" for all cases. I'm still open to passing -1 for
> > kernel held is there is a use case for it, but I don't see one.
> >
>
> Using -1 sounds good but I've just realized there's a different
> problem. A process holding a file descriptor may fork and both the
> parent and the child will keep the same file descriptors open. Now
> we'll have two processes (with different PIDs) holding the same GPIO
> lines (specifically holding a file descriptor to the same anonymous
> inode).
>
> This already poses a problem for this patch as we'd need to return an
> array of PIDs which we don't have the space for but also is a
> situation which we haven't discussed previously IIRC - two processes
> keeping the same GPIO lines requested.
>
> I don't have any good idea on how to address this yet. One thing off
> the top of my head is: close the parent's file descriptor from kernel
> space (is it even possible?) on fork() (kind of like the close() on
> exec flag).
>
> I need to think about it more.
>
I thought the O_CLOEXEC was set on the request fds exactly to prevent this
case - only one process can hold the request fd.
Cheers,
Kent.
Powered by blists - more mailing lists