Skip to content

Commit

Permalink
Linux 6.2 compatibility: Disable -funsigned-char
Browse files Browse the repository at this point in the history
As of Linux 6.2, -funsigned-char is set by the build system to change
how C compilers interpret the char type so that type promotion to an int
will always result in a positive number. This differs from the universal
meaning of char to be a signed type, such that promotion of a char to an
int will sign extend the value to ensure that negative values stay
negative.

Linux is doing this to workaround bugs involving extended ASCII
handling.  Extended ASCII is supposed to use the range from [0, 255],
but sloppy code that places it into a char makes that into the range
[-128, 127]. This by itself is fine, but when the bit width of a char
containing extended ASCII is widened to an int, the value remains in the
range [-128, 127] while code for processing it in an int only works when
it is in the range [0, 255]. Instead of finding all affected code and
fixing it, Linux has elected to tell the C compiler to treat all `char`
values as unsigned, which fixes the buggy code, but can break code that
relies on negative numbers remaining negative when widened from a char
to an int.

I used Clang Tidy to identify all places in which we have code widen
from a char to an int via its poorly named bugprone-signed-char-misuse
check. At the time of this patch, all instances of a char being widened
are at the following locations:

module/lua/lstrlib.c @ Line 342
module/lua/lstrlib.c @ Line 343
module/nvpair/nvpair.c @ Line 2630
module/nvpair/nvpair.c @ Line 2631
module/unicode/u8_textprep.c @ Line 368
module/unicode/u8_textprep.c @ Line 584
module/unicode/u8_textprep.c @ Line 621
module/unicode/u8_textprep.c @ Line 1289
module/unicode/u8_textprep.c @ Line 1424
module/unicode/u8_textprep.c @ Line 1492
module/unicode/u8_textprep.c @ Line 1518
module/unicode/u8_textprep.c @ Line 1619
module/unicode/u8_textprep.c @ Line 1984
module/zfs/dsl_deleg.c @ Line 138

Visual inspection confirms that all of them handle how C does type
promotion correctly, with one exception:

module/unicode/u8_textprep.c @ Line 1289

Here, we have:

size = u8_number_of_bytes[*p];
if (size <= 1 || (p + size) > oslast)
	break;

In that case, the type promotion is from char to size_t, which is
unsigned. C will still sign extend as part of the widening before
treating the value as unsigned and the negative values we can counter
are error values from U8_ILLEGAL_CHAR and U8_OUT_OF_RANGE_CHAR, which
are -1 and -2 respectively. The unsigned versions of these under two's
complement are SIZE_MAX and SIZE_MAX-1 respectively.

The bounds check is written under the assumption that `size <= 1` does a
signed comparison. This is followed by a pointer comparison to see if
the string has the correct length, which is fine.

A little further down we have:

for (i = 0; i < size; i++)
	tc[i] = *p++;

When an error condition is encountered, this will attempt to iterate at
least SIZE_MAX-1 times, which will massively overflow the buffer, which
is not fine. This would undoubtedly cause horrible memory corruption,
which would likely crash the system, but could cause just about anything
until the system crashes.

That said, it is desireable to preserve the existing behavior, lest we
have even more bugs, so we disallow the code to be built as a kernel
module with -funsigned-char on Linux. This also prevents us from having
to worry about the behavior of C compilers differing between user space
and kernel space, or across platforms.

A patch to fix the bug identified in module/unicode/u8_textprep.c will
be written separately.

Signed-off-by: Richard Yao <richard.yao@alumni.stonybrook.edu>
  • Loading branch information
ryao committed Dec 23, 2022
1 parent c935fe2 commit 6b4d443
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions module/Kbuild.in
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ endif
# Suppress unused-value warnings in sparc64 architecture headers
ccflags-$(CONFIG_SPARC64) += -Wno-unused-value

# Linux 6.2 compat: Do not permit Linux to insert -funsigned-char into CFLAGS.
# That flag breaks code that expects the universal interpretation of char as a
# signed type, and would cause the compiler to miscompile our code.
KBUILD_CFLAGS := $(filter-out -funsigned-char,$(KBUILD_CFLAGS))

obj-$(CONFIG_ZFS) := spl.o zfs.o

Expand Down

0 comments on commit 6b4d443

Please sign in to comment.