| 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: <CAMpxmJXOrDLdw6ZPBHxzsDRYiLmhRNCb-s_Z=Gu=Ecg1XA5ONQ@mail.gmail.com>
Date: Mon, 8 Jul 2019 11:44:56 +0200
From: Bartosz Golaszewski <bgolaszewski@...libre.com>
To: Geert Uytterhoeven <geert+renesas@...der.be>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Alexander Graf <agraf@...e.de>,
Peter Maydell <peter.maydell@...aro.org>,
Paolo Bonzini <pbonzini@...hat.com>,
Magnus Damm <magnus.damm@...il.com>,
linux-gpio <linux-gpio@...r.kernel.org>, qemu-devel@...gnu.org,
Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver
pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <geert+renesas@...der.be> napisał(a):
>
> GPIO controllers are exported to userspace using /dev/gpiochip*
> character devices. Access control to these devices is provided by
> standard UNIX file system permissions, on an all-or-nothing basis:
> either a GPIO controller is accessible for a user, or it is not.
> Currently no mechanism exists to control access to individual GPIOs.
>
> Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32),
> and expose them as a new gpiochip. This is useful for implementing
> access control, and assigning a set of GPIOs to a specific user.
> Furthermore, it would simplify and harden exporting GPIOs to a virtual
> machine, as the VM can just grab the full virtual GPIO controller, and
> no longer needs to care about which GPIOs to grab and which not,
> reducing the attack surface.
>
> Virtual GPIO controllers are instantiated by writing to the "new_device"
> attribute file in sysfs:
>
> $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]"
> "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]"
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> Likewise, virtual GPIO controllers can be destroyed after use:
>
> $ echo gpio-virt-agg.<N> \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
> ---
> Aggregating GPIOs and exposing them as a new gpiochip was suggested in
> response to my proof-of-concept for GPIO virtualization with QEMU[1][2].
>
> Sample session on r8a7791/koelsch:
>
> - Disable the leds node in arch/arm/boot/dts/r8a7791-koelsch.dts
>
> - Create virtual aggregators:
>
> $ echo "e6052000.gpio 19 20" \
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> gpio-virt-agg gpio-virt-agg.0: GPIO 0 => e6052000.gpio/19
> gpio-virt-agg gpio-virt-agg.0: GPIO 1 => e6052000.gpio/20
> gpiochip_find_base: found new base at 778
> gpio gpiochip8: (gpio-virt-agg.0): added GPIO chardev (254:8)
> gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 (gpio-virt-agg.0)
>
> $ echo "e6052000.gpio 21, e6050000.gpio 20 21 22" \
> > /sys/bus/platform/drivers/gpio-virt-agg/new_device
>
> gpio-virt-agg gpio-virt-agg.1: GPIO 0 => e6052000.gpio/21
> gpio-virt-agg gpio-virt-agg.1: GPIO 1 => e6050000.gpio/20
> gpio-virt-agg gpio-virt-agg.1: GPIO 2 => e6050000.gpio/21
> gpio-virt-agg gpio-virt-agg.1: GPIO 3 => e6050000.gpio/22
> gpiochip_find_base: found new base at 774
> gpio gpiochip9: (gpio-virt-agg.1): added GPIO chardev (254:9)
> gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 (gpio-virt-agg.1)
>
> - Adjust permissions on /dev/gpiochip[89] (optional)
>
> - Control LEDs:
>
> $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
> $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
> $ gpioset gpiochip9 0=0 # LED8 OFF
> $ gpioset gpiochip9 0=1 # LED8 ON
>
> - Destroy virtual aggregators:
>
> $ echo gpio-virt-agg.0 \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
> $ echo gpio-virt-agg.1 \
> > /sys/bus/platform/drivers/gpio-virt-agg/delete_device
>
> Thanks for your comments!
>
Hi Geert,
I like the general idea and the interface looks mostly fine. Since
this is new ABI I think it needs to be documented as well.
I'm having trouble building this module:
CALL scripts/atomic/check-atomics.sh
CALL scripts/checksyscalls.sh
CHK include/generated/compile.h
Kernel: arch/arm/boot/Image is ready
Building modules, stage 2.
MODPOST 235 modules
ERROR: "gpiod_request" [drivers/gpio/gpio-virt-agg.ko] undefined!
ERROR: "gpiochip_get_desc" [drivers/gpio/gpio-virt-agg.ko] undefined!
ERROR: "gpiod_free" [drivers/gpio/gpio-virt-agg.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1287: recipe for target 'modules' failed
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....
I'm not sure what the problem is.
> References:
> - [1] "[PATCH QEMU POC] Add a GPIO backend"
> (https://lore.kernel.org/linux-renesas-soc/20181003152521.23144-1-geert+renesas@glider.be/)
> - [2] "Getting To Blinky: Virt Edition / Making device pass-through
> work on embedded ARM"
> (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)
> ---
> drivers/gpio/Kconfig | 9 +
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-virt-agg.c | 390 +++++++++++++++++++++++++++++++++++
> 3 files changed, 400 insertions(+)
> create mode 100644 drivers/gpio/gpio-virt-agg.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index f1f02dac324e52b6..8aff4d9626dee110 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1475,3 +1475,12 @@ config GPIO_MOCKUP
> it.
>
> endif
> +
> +config GPIO_VIRT_AGG
> + tristate "GPIO Virtual Aggregator"
> + depends on GPIOLIB
> + help
> + This enabled the GPIO Virtual Aggregator, which provides a way to
> + aggregate existing GPIOs into a new virtual GPIO device.
> + This is useful for assigning a collection of GPIOs to a user, or
> + exported them to a virtual machine.
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 0a494052c1e845ee..32e885b7f3aa4eee 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -152,6 +152,7 @@ obj-$(CONFIG_GPIO_UCB1400) += gpio-ucb1400.o
> obj-$(CONFIG_GPIO_UNIPHIER) += gpio-uniphier.o
> obj-$(CONFIG_GPIO_VF610) += gpio-vf610.o
> obj-$(CONFIG_GPIO_VIPERBOARD) += gpio-viperboard.o
> +obj-$(CONFIG_GPIO_VIRT_AGG) += gpio-virt-agg.o
> obj-$(CONFIG_GPIO_VR41XX) += gpio-vr41xx.o
> obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o
> obj-$(CONFIG_GPIO_WHISKEY_COVE) += gpio-wcove.o
> diff --git a/drivers/gpio/gpio-virt-agg.c b/drivers/gpio/gpio-virt-agg.c
> new file mode 100644
> index 0000000000000000..20e5f22beed9d385
> --- /dev/null
> +++ b/drivers/gpio/gpio-virt-agg.c
> @@ -0,0 +1,390 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// GPIO Virtual Aggregator
> +//
> +// Copyright (C) 2019 Glider bvba
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +
> +#include "gpiolib.h"
> +
> +#define DRV_NAME "gpio-virt-agg"
> +#define MAX_GPIOS 32
Do we really need this limit? I see it simplifies the code, but maybe
we can allocate the relevant arrays dynamically and not limit users?
> +
> +struct gpio_virt_agg_entry {
> + struct platform_device *pdev;
> +};
> +
> +struct gpio_virt_agg_priv {
> + struct gpio_chip chip;
> + struct gpio_desc *desc[MAX_GPIOS];
> +};
> +
> +static DEFINE_MUTEX(gpio_virt_agg_lock); /* protects idr */
> +static DEFINE_IDR(gpio_virt_agg_idr);
> +
> +static int gpio_virt_agg_get_direction(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_get_direction(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_direction_input(struct gpio_chip *chip,
> + unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_direction_input(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_direction_output(struct gpio_chip *chip,
> + unsigned int offset, int value)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_direction_output(priv->desc[offset], value);
> +}
> +
> +static int gpio_virt_agg_get(struct gpio_chip *chip, unsigned int offset)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + return gpiod_get_value(priv->desc[offset]);
> +}
> +
> +static int gpio_virt_agg_get_multiple(struct gpio_chip *chip,
> + unsigned long *mask, unsigned long *bits)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + DECLARE_BITMAP(values, MAX_GPIOS) = { 0 };
> + struct gpio_desc *desc[MAX_GPIOS];
> + unsigned int i, j = 0;
> + int ret;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio)
> + desc[j++] = priv->desc[i];
> +
> + ret = gpiod_get_array_value(j, desc, NULL, values);
> + if (ret)
> + return ret;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio)
> + __assign_bit(i, bits, test_bit(j++, values));
> +
> + return 0;
> +}
> +
> +static void gpio_virt_agg_set(struct gpio_chip *chip, unsigned int offset,
> + int value)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + gpiod_set_value(priv->desc[offset], value);
> +}
> +
> +static void gpio_virt_agg_set_multiple(struct gpio_chip *chip,
> + unsigned long *mask,
> + unsigned long *bits)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + DECLARE_BITMAP(values, MAX_GPIOS);
> + struct gpio_desc *desc[MAX_GPIOS];
> + unsigned int i, j = 0;
> +
> + for_each_set_bit(i, mask, priv->chip.ngpio) {
> + __assign_bit(j, values, test_bit(i, bits));
> + desc[j++] = priv->desc[i];
> + }
> +
> + gpiod_set_array_value(j, desc, NULL, values);
> +}
> +
> +static int gpio_virt_agg_set_config(struct gpio_chip *chip,
> + unsigned int offset, unsigned long config)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> +
> + chip = priv->desc[offset]->gdev->chip;
> + if (chip->set_config)
> + return chip->set_config(chip, offset, config);
> +
> + // FIXME gpiod_set_transitory() expects success if not implemented
> + return -ENOTSUPP;
> +}
> +
> +static int gpio_virt_agg_init_valid_mask(struct gpio_chip *chip)
> +{
> + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip);
> + unsigned int i;
> +
> + for (i = 0; i < priv->chip.ngpio; i++) {
> + if (gpiochip_line_is_valid(priv->desc[i]->gdev->chip,
> + gpio_chip_hwgpio(priv->desc[i])))
> + set_bit(i, chip->valid_mask);
> + }
> +
> + return 0;
> +}
> +
> +static int gpiochip_match_label(struct gpio_chip *chip, void *data)
> +{
> + return !strcmp(chip->label, data);
> +}
> +
> +static struct gpio_chip *gpiochip_find_by_label(const char *label)
> +{
> + return gpiochip_find((void *)label, gpiochip_match_label);
> +}
> +
> +static ssize_t new_device_store(struct device_driver *driver, const char *buf,
> + size_t count)
> +{
> + struct gpio_virt_agg_entry *gva;
> + struct platform_device *pdev;
> + int res, id;
> +
> + gva = kzalloc(sizeof(*gva), GFP_KERNEL);
> + if (!gva)
> + return -ENOMEM;
> +
> + mutex_lock(&gpio_virt_agg_lock);
> + id = idr_alloc(&gpio_virt_agg_idr, gva, 0, 0, GFP_KERNEL);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + if (id < 0) {
> + res = id;
> + goto free_gva;
> + }
> +
> + /* kernfs guarantees string termination, so count + 1 is safe */
> + pdev = platform_device_register_data(NULL, DRV_NAME, id, buf,
> + count + 1);
> + if (IS_ERR(pdev)) {
> + res = PTR_ERR(pdev);
> + goto remove_idr;
> + }
> +
> + gva->pdev = pdev;
> + return count;
> +
> +remove_idr:
> + mutex_lock(&gpio_virt_agg_lock);
> + idr_remove(&gpio_virt_agg_idr, id);
> + mutex_unlock(&gpio_virt_agg_lock);
> +free_gva:
> + kfree(gva);
> + return res;
> +}
> +
> +static DRIVER_ATTR_WO(new_device);
> +
> +static ssize_t delete_device_store(struct device_driver *driver,
> + const char *buf, size_t count)
> +{
> + struct gpio_virt_agg_entry *gva;
> + int id;
> +
> + if (strncmp(buf, DRV_NAME ".", strlen(DRV_NAME ".")))
> + return -EINVAL;
> +
> + id = simple_strtoul(buf + strlen(DRV_NAME "."), NULL, 10);
> +
> + mutex_lock(&gpio_virt_agg_lock);
> + gva = idr_remove(&gpio_virt_agg_idr, id);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + if (!gva) {
> + pr_info("Cannot find %s.%d\n", DRV_NAME, id);
> + return -ENOENT;
> + }
> +
> + platform_device_unregister(gva->pdev);
> + kfree(gva);
> + return count;
> +}
> +static DRIVER_ATTR_WO(delete_device);
> +
> +static struct attribute *gpio_virt_agg_attrs[] = {
> + &driver_attr_new_device.attr,
> + &driver_attr_delete_device.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(gpio_virt_agg);
> +
> +static int gpio_virt_agg_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + const char *param = dev_get_platdata(dev);
> + struct gpio_virt_agg_priv *priv;
> + const char *label = NULL;
> + struct gpio_chip *chip;
> + struct gpio_desc *desc;
> + unsigned int offset;
> + int error, i;
This 'i' here is reported as possibly not initialized:
drivers/gpio/gpio-virt-agg.c: In function ‘gpio_virt_agg_probe’:
drivers/gpio/gpio-virt-agg.c:230:13: warning: ‘i’ may be used
uninitialized in this function [-Wmaybe-uninitialized]
int error, i;
^
> + char *s;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + error = -ENOMEM;
> + goto fail;
> + }
> +
> + for (i = 0; i < MAX_GPIOS; i++) {
> + if (*param == '\0' || *param == '\n')
> + break;
> +
> + if (*param == ',') {
> + if (label) {
> + devm_kfree(dev, label);
> + label = NULL;
> + }
> + for (param++; *param == ' '; param++) ;
> + }
> +
> + if (!label) {
> + s = strchr(param, ' ');
> + if (!s) {
> + dev_info(dev, "Missing gpiochip\n");
> + error = -EINVAL;
> + goto fail;
> + }
> + label = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> + (int)(s - param), param);
> + if (!label) {
> + error = -ENOMEM;
> + goto fail;
> + }
> +
> + chip = gpiochip_find_by_label(label);
> + if (!chip) {
> + dev_info(dev, "Cannot find gpiochip %s\n",
> + label);
> + error = -ENODEV;
> + goto fail;
> + }
> +
> + for (param = s + 1; *param == ' '; param++) ;
> + }
> +
> + offset = simple_strtoul(param, &s, 10);
> +
> + desc = gpiochip_get_desc(chip, offset);
> + if (IS_ERR(desc)) {
> + error = PTR_ERR(desc);
> + dev_info(dev, "Cannot get GPIO %s/%u: %d\n", label,
> + offset, error);
> + goto fail;
> + }
> +
> + error = gpiod_request(desc, dev_name(dev));
> + if (error) {
> + dev_info(dev, "Cannot request GPIO %s/%u: %d\n", label,
> + offset, error);
> + goto fail;
> + }
> +
> + dev_dbg(dev, "GPIO %u => %s/%u\n", i, label, offset);
> + priv->desc[i] = desc;
> +
> + if (gpiod_cansleep(desc))
> + priv->chip.can_sleep = true;
> + if (desc->gdev->chip->set_config)
> + priv->chip.set_config = gpio_virt_agg_set_config;
> + if (desc->gdev->chip->need_valid_mask) {
> + priv->chip.need_valid_mask = true;
> + priv->chip.init_valid_mask =
> + gpio_virt_agg_init_valid_mask;
> + }
> +
> + for (param = s; *param == ' '; param++) ;
> + }
> + if (i == MAX_GPIOS)
> + dev_warn(&pdev->dev,
> + "Too many gpios specified, truncating to %u\n",
> + MAX_GPIOS);
> +
> + priv->chip.label = dev_name(dev);
> + priv->chip.parent = dev;
> + priv->chip.owner = THIS_MODULE;
> + priv->chip.get_direction = gpio_virt_agg_get_direction;
> + priv->chip.direction_input = gpio_virt_agg_direction_input;
> + priv->chip.direction_output = gpio_virt_agg_direction_output;
> + priv->chip.get = gpio_virt_agg_get;
> + priv->chip.get_multiple = gpio_virt_agg_get_multiple;
> + priv->chip.set = gpio_virt_agg_set;
> + priv->chip.set_multiple = gpio_virt_agg_set_multiple;
> + priv->chip.base = -1;
> + priv->chip.ngpio = i;
> + platform_set_drvdata(pdev, priv);
> +
> + error = gpiochip_add_data(&priv->chip, priv);
> + if (error)
> + goto fail;
> +
> + return 0;
> +
> +fail:
> + while (i-- > 0)
> + gpiod_free(priv->desc[i]);
> +
> + return error;
> +}
> +
> +static int gpio_virt_agg_remove(struct platform_device *pdev)
> +{
> + struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev);
> + unsigned int i;
> +
> + gpiochip_remove(&priv->chip);
> +
> + for (i = 0; i < priv->chip.ngpio; i++)
> + gpiod_free(priv->desc[i]);
> +
> + return 0;
> +}
You shouldn't need this function at all. It's up to users to free descriptors.
Best regards,
Bartosz Golaszewski
> +
> +static struct platform_driver gpio_virt_agg_driver = {
> + .probe = gpio_virt_agg_probe,
> + .remove = gpio_virt_agg_remove,
> + .driver = {
> + .name = DRV_NAME,
> + .groups = gpio_virt_agg_groups,
> + },
> +};
> +
> +static int __init gpio_virt_agg_init(void)
> +{
> + return platform_driver_register(&gpio_virt_agg_driver);
> +}
> +module_init(gpio_virt_agg_init);
> +
> +static int __exit gpio_virt_agg_idr_remove(int id, void *p, void *data)
> +{
> + struct gpio_virt_agg_entry *gva = p;
> +
> + platform_device_unregister(gva->pdev);
> + kfree(gva);
> + return 0;
> +}
> +
> +static void __exit gpio_virt_agg_exit(void)
> +{
> + mutex_lock(&gpio_virt_agg_lock);
> + idr_for_each(&gpio_virt_agg_idr, gpio_virt_agg_idr_remove, NULL);
> + idr_destroy(&gpio_virt_agg_idr);
> + mutex_unlock(&gpio_virt_agg_lock);
> +
> + platform_driver_unregister(&gpio_virt_agg_driver);
> +}
> +module_exit(gpio_virt_agg_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@...der.be>");
> +MODULE_DESCRIPTION("GPIO Virtual Aggregator");
> +MODULE_LICENSE("GPL v2");
> --
> 2.17.1
>
Powered by blists - more mailing lists