lkml.org 
[lkml]   [2022]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 00/12] Clang -Wformat warning fixes
    On Thu, Jun 9, 2022 at 3:25 PM Andrew Morton <akpm@linux-foundation.org> wrote:
    >
    > On Thu, 9 Jun 2022 22:16:19 +0000 Bill Wendling <morbo@google.com> wrote:
    >
    > > This patch set fixes some clang warnings when -Wformat is enabled.

    It looks like this series fixes -Wformat-security, which while being a
    member of the -Wformat group, is intentionally disabled in the kernel
    and somewhat orthogonal to enabling -Wformat with Clang.

    -Wformat is a group flag (like -Wall) that enables multiple other
    flags implicitly. Reading through
    clang/include/clang/Basic/DiagnosticGroups.td in clang's sources, it
    looks like:

    1. -Wformat is a group flag.
    2. -Wformat-security is a member of the -Wformat group; enabling
    -Wformat will enable -Wformat-security.
    3. -Wformat itself is a member of -Wmost (never heard of -Wmost, but
    w/e). So -Wmost will enable -Wformat will enable -Wformat-security.
    4. -Wmost is itself a member of -Wall. -Wall enables -Wmost enables
    -Wformat enables -Wformat security.

    Looking now at Kbuild:
    1. Makefile:523 adds -Wall to KBUILD_CFLAGS.
    2. The same assignment expression but on line 526 immediately disables
    -Wformat-security via -Wno-format-security.
    3. scripts/Makefile.extrawarn disables -Wformat via -Wno-format only
    for clang (via guard of CONFIG_CC_IS_CLANG).

    We _want_ -Wformat enabled for clang so that developers aren't sending
    patches that trigger -Wformat with GCC (if they didn't happen to test
    their code with both). It's disabled for clang until we can build the
    kernel cleanly with it enabled, which we'd like to do.

    I don't think that we need to enable -Wformat-security to build with
    -Wformat for clang.

    I suspect based on Randy's comment on patch 1/12 that perhaps -Wformat
    was _added_ to KBUILD_CFLAGS in scripts/Makefile.extrawarn rather than
    -Wno-format being _removed_. The former would re-enable
    -Wformat-security due to the grouping logic described above. The
    latter is probably closer to our ultimate goal of enabling -Wformat
    coverage for clang (or rather not disabling the coverage via
    -Wno-format; a double negative).

    I'm pretty sure the kernel doesn't support %n in format strings...see
    the comment above vsnprintf in lib/vsprintf.c. Are there other
    attacks other than %n that -Wformat-security guards against? Maybe
    there's some context on the commit that added -Wno-format-security to
    the kernel? Regardless, I don't think enabling -Wformat-security is a
    blocker for enabling -Wformat (or...disabling -Wno-format...two sides
    of the same coin) for clang.

    > >
    >
    > tldr:
    >
    > - printk(msg);
    > + printk("%s", msg);
    >
    > the only reason to make this change is where `msg' could contain a `%'.
    > Generally, it came from userspace. Otherwise these changes are a
    > useless consumer of runtime resources.
    >
    > I think it would be better to quieten clang in some fashion.



    --
    Thanks,
    ~Nick Desaulniers

    \
     
     \ /
      Last update: 2022-06-10 02:34    [W:3.847 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site