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.