| 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
| ||
|
Date: Wed, 23 Sep 2020 14:11:54 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Kent Gibson <warthog618@...il.com>
Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
"open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Linus Walleij <linus.walleij@...aro.org>,
Arnd Bergmann <arnd@...db.de>
Subject: Re: [PATCH v9 07/20] gpiolib: cdev: support GPIO_V2_GET_LINE_IOCTL
and GPIO_V2_LINE_GET_VALUES_IOCTL
On Tue, Sep 22, 2020 at 5:35 AM Kent Gibson <warthog618@...il.com> wrote:
>
> Add support for requesting lines using the GPIO_V2_GET_LINE_IOCTL, and
> returning their current values using GPIO_V2_LINE_GET_VALUES_IOCTL.
>
> The struct linereq implementation is based on the v1 struct linehandle
> implementation.
...
> + /*
> + * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If
You see, in some cases you are using "OR:ed" as understandable for
programmers, and here & which should be and in plain English and
really confusing from a programmer's perspective. That's why I prefer
to see plain English rather than something which is full of encoded
meanings.
> + * the hardware actually supports enabling both at the same time the
> + * electrical result would be disastrous.
> + */
...
> + /* Bias requires explicit direction. */
> + if ((flags & GPIO_V2_LINE_BIAS_FLAGS) &&
> + !(flags & GPIO_V2_LINE_DIRECTION_FLAGS))
> + return -EINVAL;
Okay, since this is strict we probably may relax it in the future if
it will be a use case.
...
> + /* Only one bias flag can be set. */
Ditto. (Some controllers allow to set both simultaneously, though I
can't imagine good use case for that)
> + if (((flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED) &&
> + (flags & (GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN |
> + GPIO_V2_LINE_FLAG_BIAS_PULL_UP))) ||
> + ((flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN) &&
> + (flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP)))
> + return -EINVAL;
...
> +static void gpio_v2_line_config_flags_to_desc_flags(u64 flags,
> + unsigned long *flagsp)
> +{
> + assign_bit(FLAG_ACTIVE_LOW, flagsp,
> + flags & GPIO_V2_LINE_FLAG_ACTIVE_LOW);
What I meant is to attach also this to the other assign_bit():s below.
And just in case a question: why not __asign_bit() do we really need atomicity?
> + if (flags & GPIO_V2_LINE_FLAG_OUTPUT)
> + set_bit(FLAG_IS_OUT, flagsp);
> + else if (flags & GPIO_V2_LINE_FLAG_INPUT)
> + clear_bit(FLAG_IS_OUT, flagsp);
> +
> + assign_bit(FLAG_OPEN_DRAIN, flagsp,
> + flags & GPIO_V2_LINE_FLAG_OPEN_DRAIN);
> + assign_bit(FLAG_OPEN_SOURCE, flagsp,
> + flags & GPIO_V2_LINE_FLAG_OPEN_SOURCE);
> + assign_bit(FLAG_PULL_UP, flagsp,
> + flags & GPIO_V2_LINE_FLAG_BIAS_PULL_UP);
> + assign_bit(FLAG_PULL_DOWN, flagsp,
> + flags & GPIO_V2_LINE_FLAG_BIAS_PULL_DOWN);
> + assign_bit(FLAG_BIAS_DISABLE, flagsp,
> + flags & GPIO_V2_LINE_FLAG_BIAS_DISABLED);
> +}
...
> +static long linereq_get_values(struct linereq *lr, void __user *ip)
> +{
> + struct gpio_v2_line_values lv;
> + DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX);
> + struct gpio_desc **descs;
> + unsigned int i, didx, num_get;
> + int ret;
> + /* NOTE: It's ok to read values of output lines. */
> + if (copy_from_user(&lv, ip, sizeof(lv)))
> + return -EFAULT;
> +
> + for (num_get = 0, i = 0; i < lr->num_lines; i++) {
> + if (lv.mask & BIT_ULL(i)) {
> + num_get++;
> + descs = &lr->lines[i].desc;
> + }
> + }
So what you can do here is something like
DECLARE_BITMAP(mask, u64);
...
bitmap_from_u64(mask, lv.mask);
num_get = bitmap_weight(mask, lr->num_lines);
if (num_get == 0)
return -EINVAL;
for_each_set_bit(i, mask, lr->num_lines)
descs = &lr->lines[i].desc;
// I'm not sure I understood a purpose of the above
// ah, looks like malloc() avoidance, but you may move it below...
> + if (num_get == 0)
> + return -EINVAL;
> +
> + if (num_get != 1) {
...something like
if (num_get == 1)
descs = ...[find_first_bit(mask, lr->num_lines)];
else {
...
for_each_set_bit() {
...
}
}
> + descs = kmalloc_array(num_get, sizeof(*descs), GFP_KERNEL);
> + if (!descs)
> + return -ENOMEM;
> + for (didx = 0, i = 0; i < lr->num_lines; i++) {
> + if (lv.mask & BIT_ULL(i)) {
> + descs[didx] = lr->lines[i].desc;
> + didx++;
> + }
> + }
> + }
> + ret = gpiod_get_array_value_complex(false, true, num_get,
> + descs, NULL, vals);
> +
> + if (num_get != 1)
> + kfree(descs);
> + if (ret)
> + return ret;
> +
> + lv.bits = 0;
> + for (didx = 0, i = 0; i < lr->num_lines; i++) {
> + if (lv.mask & BIT_ULL(i)) {
> + if (test_bit(didx, vals))
> + lv.bits |= BIT_ULL(i);
> + didx++;
> + }
> + }
So here...
> + if (copy_to_user(ip, &lv, sizeof(lv)))
> + return -EFAULT;
> +
> + return 0;
> +}
...
> + /* Make sure this is terminated */
> + ulr.consumer[sizeof(ulr.consumer)-1] = '\0';
> + if (strlen(ulr.consumer)) {
> + lr->label = kstrdup(ulr.consumer, GFP_KERNEL);
> + if (!lr->label) {
> + ret = -ENOMEM;
> + goto out_free_linereq;
> + }
> + }
Still don't get why we can\t use kstrndup() here...
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists