linux-kernel - Re: [PATCH] gpio: Document the 'valid

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdVLqS0=OXBMPAct9bkNWcRHTEN49v0uUiZdK8M-hmRKxw@mail.gmail.com>
Date: Fri, 28 Feb 2025 10:32:22 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Matti Vaittinen <mazziesaccount@...il.com>, 
	Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, Bartosz Golaszewski <brgl@...ev.pl>, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Biju Das <biju.das.jz@...renesas.com>
Subject: Re: [PATCH] gpio: Document the 'valid_mask' being internal

Hi Linus,

CC Biju

On Fri, 28 Feb 2025 at 09:07, Linus Walleij <linus.walleij@...aro.org> wrote:
> On Wed, Feb 26, 2025 at 12:42 PM Matti Vaittinen
> <mazziesaccount@...il.com> wrote:
> > On 26/02/2025 12:18, Linus Walleij wrote:
> > > That's easy to check with some git grep valid_mask
> >
> > True. I just tried. It seems mostly Ok, but...
> > For example the drivers/gpio/gpio-rcar.c uses the contents of the
> > 'valid_mask' in it's set_multiple callback to disallow setting the value
> > of masked GPIOs.
> >
> > For uneducated person like me, it feels this check should be done and
> > enforced by the gpiolib and not left for untrustworthy driver writers
> > like me! (I am working on BD79124 driver and it didn't occur to me I
> > should check for the valid_mask in driver :) If gpiolib may call the
> > driver's set_multiple() with masked lines - then the bd79124 driver just
> > had one unknown bug less :rolleyes:) )
>
> Yeah that should be done in gpiolib.
>
> And I think it is, gpiolib will not allow you to request a line
> that is not valid AFAIK.

Correct, since commit 3789f5acb9bbe088 ("gpiolib: Avoid calling
chip->request() for unused gpios") by Biju.

> This check in rcar is just overzealous and can probably be
> removed. Geert what do you say?

I looked at the history, and the related discussion.  It was actually
Biju who added the valid_mask check to gpio_rcar_set_multiple()
(triggering the creation of commit 3789f5acb9bbe088), and I just copied
that when adding gpio_rcar_get_multiple().
His v2[1] had checks in both the .request() and .set_multiple()
callbacks, but it's possible he added the latter first, and didn't
realize that became unneeded after adding the former.  The final version
v3[2] retained only the check in .set_multiple(), as by that time the
common gpiod_request_commit() had gained a check.

While .set_multiple() takes hardware offsets, not gpio_desc pointers,
these do originate from an array of gpio_desc pointers, so all of them
must have been requested properly.
We never exposed set_multiple() with raw GPIO numbers to users, right?
So I agree the check is probably not needed.

[1] https://lore.kernel.org/linux-renesas-soc/1533219087-33695-2-git-send-email-biju.das@bp.renesas.com
[2] https://lore.kernel.org/linux-renesas-soc/1533628626-26503-2-git-send-email-biju.das@bp.renesas.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds