CONFIG_FORTIFY_SOURCE may not be working for strcpy() and strlcpy() on LoongArch with clang

[nathanchance]

When building ARCH=loongarch allyesconfig with the known bad configurations disabled, I see some warnings from the fortify tests that strcpy() and strlcpy() do not have a fortify check when they should:

$ make -skj"$(nproc)" ARCH=loongarch KCONFIG_ALLCONFIG=<(echo 'CONFIG_MODULES=n
CONFIG_CRASH_DUMP=n
CONFIG_RELOCATABLE=n
CONFIG_WERROR=n') LLVM=1 mrproper allyesconfig lib/
warning: unsafe strcpy() usage lacked '__write_overflow' symbol in $LINUX/lib/test_fortify/write_overflow-strcpy-lit.c
warning: unsafe strcpy() usage lacked '__write_overflow' symbol in $LINUX/lib/test_fortify/write_overflow-strcpy.c
warning: unsafe strlcpy() usage lacked '__write_overflow' symbol in $LINUX/lib/test_fortify/write_overflow-strlcpy-src.c
warning: unsafe strlcpy() usage lacked '__write_overflow' symbol in $LINUX/lib/test_fortify/write_overflow-strlcpy.c

I don't see any warnings with GCC 13.1.0 but I also do not see these warnings with any other architecture, which is interesting. It is possible there is some bug on the toolchain side or something about the tests that allows LoongArch to avoid the checks? I'll see if I can gather more information on Monday.

cc @kees @xen0n

[nathanchance]

This appears to be related to LoongArch's use of -ffreestanding, according to my initial reduction of the compiler flags to create a small test case, as this issue does not occur when -ffreestanding is removed from the compiler invocation.

It has been there since LoongArch's introduction in https://git.kernel.org/linus/fa96b57c149061f71a70bd6582d995f6424fbbf4 and I see some upstream discussion around it at https://lore.kernel.org/20230712101344.2714626-1-chenhuacai@loongson.cn/.

[nickdesaulniers]

<personal opinion>: it is a mistake to use -ffreestanding in the Linux kernel, and especially egregious that it's sometimes used or not per architecture. I intend to remove it from i386 now that the llvm bug at the heart of #1583 has been fixed. Other architectures should also avoid -ffreestanding; generally the Linux kernel does a good job of providing the same symbols with the same semantics as libc (for common simple routines); by using -ffreestanding those architectures miss out on all kinds of compiler optimizations that take advantage of the specific semantics of libc functions.
</rant>
But I could be convinced otherwise.

It make sense perhaps to see about --rtlib=none (no compiler runtime exists in the runtime environment), but not -ffreestanding (IMO).

I'd be curious if -ffreestanding and -D_FORTIFY_SOURCE= has positional dependence on codegen between compilers. If -ffreestanding doesn't disable fortify in GCC, that would be surprising to me (possible given the above report), and we should consider if that's a bug in GCC or behavior that clang aught to match.

[nathanchance]

cvise kept producing invalid reproducers so I finally had it leave the actual fortify routines and tests alone and I ended up with the somewhat verbose reproducer:

typedef unsigned long __kernel_size_t;
typedef __kernel_size_t size_t;
void fortify_panic(const char *);
void __write_overflow()
	__attribute__((__error__("detected write beyond size of object 0")));
extern __kernel_size_t __real_strnlen(const char *,
				      __kernel_size_t) __asm__("strnlen");
extern inline __attribute__((__gnu_inline__))
__attribute__((patchable_function_entry(0, 0)))
__attribute__((__always_inline__)) __attribute__((__gnu_inline__))
__attribute__((__overloadable__)) __kernel_size_t
strnlen(const char *const __attribute__((__pass_dynamic_object_size__(1))) p,
	__kernel_size_t maxlen)
{
	const size_t p_size = __builtin_dynamic_object_size(p, 1);
	const size_t p_len = ({
		char *__p = (char *)(p);
		size_t __ret = (~(size_t)0);
		const size_t __p_size = __builtin_dynamic_object_size(p, 1);
		if (__p_size != (~(size_t)0) && __builtin_constant_p(*__p)) {
			size_t __p_len = __p_size - 1;
			if (__builtin_constant_p(__p[__p_len]) &&
			    __p[__p_len] == '\0')
				__ret = __builtin_strlen(__p);
		}
		__ret;
	});
	size_t ret;

	if (__builtin_constant_p(maxlen) && p_len != (~(size_t)0)) {
		if (maxlen >= p_size)
			return p_len;
	}

	ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
	if (p_size <= ret && maxlen != ret)
		fortify_panic(__func__);
	return ret;
}
extern inline __attribute__((__gnu_inline__))
__attribute__((patchable_function_entry(0, 0)))
__attribute__((__always_inline__)) __attribute__((__gnu_inline__))
__attribute__((__overloadable__))
__attribute__((__diagnose_as_builtin__(__builtin_strlen, 1))) __kernel_size_t
__fortify_strlen(const char *const
		 __attribute__((__pass_dynamic_object_size__(1))) p)
{
	const size_t p_size = __builtin_dynamic_object_size(p, 1);
	__kernel_size_t ret;

	if (p_size == (~(size_t)0))
		return __builtin_strlen(p);
	ret = strnlen(p, p_size);
	if (p_size <= ret)
		fortify_panic(__func__);
	return ret;
}

extern size_t __real_strlcpy(char *, const char *, size_t) __asm__("strlcpy");
extern inline __attribute__((__gnu_inline__))
__attribute__((patchable_function_entry(0, 0)))
__attribute__((__always_inline__)) __attribute__((__gnu_inline__))
__attribute__((__overloadable__)) size_t
strlcpy(char *const __attribute__((__pass_dynamic_object_size__(1))) p,
	const char *const __attribute__((__pass_dynamic_object_size__(1))) q,
	size_t size)
{
	const size_t p_size = __builtin_dynamic_object_size(p, 1);
	const size_t q_size = __builtin_dynamic_object_size(q, 1);
	size_t q_len;
	size_t len;

	if (p_size == (~(size_t)0) && q_size == (~(size_t)0))
		return __real_strlcpy(p, q, size);
	q_len = __builtin_choose_expr(
		(sizeof(int) ==
		 sizeof(*(8 ? ((void *)((long)(__builtin_strlen(q)) * 0l)) :
			      (int *)8))),
		__builtin_strlen(q), __fortify_strlen(q));
	len = (q_len >= size) ? size - 1 : q_len;
	if (__builtin_constant_p(size) && __builtin_constant_p(q_len) && size) {
		if (len >= p_size)
			__write_overflow();
	}
	if (size) {
		if (len >= p_size)
			fortify_panic(__func__);
		__builtin_memcpy(p, q, len);
		p[len] = '\0';
	}
	return q_len;
}
void fortify_memset_chk(__kernel_size_t, size_t, size_t);
void do_fortify_tests(void);

struct fortify_object {
	int a;
	char buf[16];
	int c;
};

const char small_src[16] = "AAAAAAAAAAAAAAA";
const char large_src[32] = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";

char small[16];
char large[32];
struct fortify_object instance;
size_t size;

void do_fortify_tests(void)
{
	({
		size_t __fortify_size = (size_t)(sizeof(instance));
		fortify_memset_chk(__fortify_size,
				   __builtin_dynamic_object_size(&instance, 0),
				   __builtin_dynamic_object_size(&instance, 1)),
			__builtin_memset(&instance, 0x32, __fortify_size);
	});
	({
		size_t __fortify_size = (size_t)(sizeof(small));
		fortify_memset_chk(__fortify_size,
				   __builtin_dynamic_object_size(small, 0),
				   __builtin_dynamic_object_size(small, 1)),
			__builtin_memset(small, 0xA5, __fortify_size);
	});
	({
		size_t __fortify_size = (size_t)(sizeof(large));
		fortify_memset_chk(__fortify_size,
				   __builtin_dynamic_object_size(large, 0),
				   __builtin_dynamic_object_size(large, 1)),
			__builtin_memset(large, 0x5A, __fortify_size);
	});

	strlcpy(instance.buf, large_src, sizeof(instance.buf) + 1);
}

Which makes it no longer strictly a LoongArch problem (although I think the use of -ffreestanding does need to be examined).

$ gcc -O2 -c -o /dev/null write_overflow-strlcpy.i
...
In function ‘strlcpy’,
    inlined from ‘do_fortify_tests’ at write_overflow-strlcpy.i:134:2:
write_overflow-strlcpy.i:83:25: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object 0
   83 |                         __write_overflow();
      |                         ^~~~~~~~~~~~~~~~~~

$ gcc -O2 -ffreestanding -c -o /dev/null write_overflow-strlcpy.i
...
In function ‘strlcpy’,
    inlined from ‘do_fortify_tests’ at write_overflow-strlcpy.i:134:2:
write_overflow-strlcpy.i:83:25: error: call to ‘__write_overflow’ declared with attribute error: detected write beyond size of object 0
   83 |                         __write_overflow();
      |                         ^~~~~~~~~~~~~~~~~~

$ clang -O2 -c -o /dev/null write_overflow-strlcpy.i
write_overflow-strlcpy.i:83:4: error: call to '__write_overflow' declared with 'error' attribute: detected write beyond size of object 0
   83 |                         __write_overflow();
      |                         ^
1 error generated.

$ clang -O2 -ffreestanding -c -o clang.o write_overflow-strlcpy.i

$ llvm-nm clang.o
0000000000000000 r .LCPI0_0
0000000000000010 r .LCPI0_1
0000000000000020 r .LCPI0_2
0000000000000008 r .L__func__._Z16__fortify_strlenPKcU25pass_dynamic_object_size1
0000000000000000 r .L__func__._Z7strlcpyPcU25pass_dynamic_object_size1PKcU25pass_dynamic_object_size1m
0000000000000019 r .L__func__._Z7strnlenPKcU25pass_dynamic_object_size1m
0000000000000000 T do_fortify_tests
                 U fortify_memset_chk
                 U fortify_panic
0000000000000000 B instance
0000000000000030 B large
0000000000000010 R large_src
0000000000000050 B size
0000000000000020 B small
0000000000000000 R small_src
                 U strlen
                 U strnlen

One of the reproducers cvise spat out (which I don't have anymore :/) makes me think this could have something to do with __builtin_constant_p?

[nickdesaulniers]

One of the reproducers cvise spat out (which I don't have anymore :/) makes me think this could have something to do with __builtin_constant_p?

Yeah, If I add:

	len = (q_len >= size) ? size - 1 : q_len;
+    if (__builtin_constant_p(q_len))
+        __write_overflow();
	if (__builtin_constant_p(size) && __builtin_constant_p(q_len) && size) {

in strlcpy, I get an additional failure that goes away with -ffreestanding.

[kees]

Hm, I'm not super excited about trying to make FORTIFY work perfectly on -ffreestanding architectures. I wonder if this should be made a Kconfig so that the selftest can skip it under those architectures?

[xen0n]

I'll attempt to just remove that -ffreestanding.

Update: apparently removing it doesn't affect operation at all. I'll put up the patch shortly...

[nathanchance]

v3: https://lore.kernel.org/20230806103620.3118683-1-kernel@xen0n.name/

That change is tentatively in -next, where I do not see these warnings anymore.

[nathanchance]

https://git.kernel.org/chenhuacai/linux-loongson/c/e2843c7596e77f4f9b006a1417546231395d0986

[nathanchance]

I think this is now fixed in mainline?

https://git.kernel.org/linus/3f301dc292eb122eff61b8b2906e519154b0327f

[xen0n]

Yeah this is now mainlined, and I think this issue can be closed now. Thanks for your help!