linux-kernel - Re: [PATCH v2] gpiolib: acpi: use correct format characters

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: <CAHk-=wgH3kbKcNwBO7os3BuU523Gd2aqdu0dVvf50bJbLKrLJg@mail.gmail.com>
Date:   Sat, 19 Mar 2022 16:21:09 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Bill Wendling <morbo@...gle.com>
Cc:     Mika Westerberg <mika.westerberg@...ux.intel.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        llvm@...ts.linux.dev
Subject: Re: [PATCH v2] gpiolib: acpi: use correct format characters

On Sat, Mar 19, 2022 at 3:54 PM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> So warning that '%hhX' is paired with an 'int' is all just completely
> mindless and wrong.

Sadly, I can see a different bogus warning reason why people would
want to use '%02hhX'.

Again, the *sane* thing from a human perspective is to use '%02X. But
if the compiler doesn't do any range analysis at all, it could decide
that "Oh, that print format could need up to 8 bytes of space in the
result". Using '%02hhX' would cut that down to two.

And since we use

        char ev_name[5];

and currently use "_%c%02hhX" as the format string, even a compiler
that doesn't notice that "pin <= 255" test that guards this all will
go "ok, that's at most 4 bytes and the final NUL termination, so it's
fine".

While a compiler - like gcc - that only sees that the original source
of the 'pin' value is a 'unsigned short' array, and then doesn't take
the "pin <= 255" into account, will warn like this:

    drivers/gpio/gpiolib-acpi.c: In function 'acpi_gpiochip_request_interrupt':
    drivers/gpio/gpiolib-acpi.c:206:24: warning: '%02X' directive
writing between 2 and 4 bytes into a region of size 3
[-Wformat-overflow=]
       sprintf(ev_name, "_%c%02X",
                            ^~~~
    drivers/gpio/gpiolib-acpi.c:206:20: note: directive argument in
the range [0, 65535]

because gcc isn't being very good at that argument range analysis either.

In other words, the original use of 'hhx' was bogus to begin with, and
due to *another* compiler warning being bad, and we had that bad code
being written back in 2016 to work around _that_ compiler warning
(commit e40a3ae1f794: "gpio: acpi: work around false-positive
-Wstring-overflow warning").

Sadly, two different bad compiler warnings together does not make for
one good one.

It just makes for even more pain.

End result: I think the simplest and cleanest option is simply this:

  --- a/drivers/gpio/gpiolib-acpi.c
  +++ b/drivers/gpio/gpiolib-acpi.c
  @@ -387,8 +387,8 @@ static acpi_status
acpi_gpiochip_alloc_event(struct acpi_resource *ares,
        pin = agpio->pin_table[0];

        if (pin <= 255) {
  -             char ev_name[5];
  -             sprintf(ev_name, "_%c%02hhX",
  +             char ev_name[8];
  +             sprintf(ev_name, "_%c%02X",
                        agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
                        pin);
                if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle)))

which undoes that '%hhX' change for gcc, and replaces it with just
using a slightly bigger stack allocation. It's not like a 5-byte
allocation is in any way likely to have saved any actual stack, since
all the other variables in that function are 'int' or bigger.

False-positive compiler warnings really do make people write worse
code, and that's a problem. But on a scale of bad code, I feel that
extending the buffer trivially is better than adding a pointless cast
that literally makes no sense.

At least in this case the end result isn't unreadable or buggy. We've
had several cases of bad compiler warnings that caused changes that
were actually horrendously wrong.

                  Linus