New fortify warning in fs/smb/client/cifsencrypt.c after LLVM commit d77067d08a3f56dc2d0e6c95bd2852c943df743a

[nathanchance]

After llvm/llvm-project@d77067d, I see a warning in fs/smb/client/cifsencrypt.c with multiple architectures.

$ make -skj"$(nproc)" ARCH=arm64 LLVM=1 mrproper allmodconfig fs/smb/client/cifsencrypt.o
In file included from fs/smb/client/cifsencrypt.c:12:
In file included from include/linux/fs.h:6:
In file included from include/linux/wait_bit.h:8:
In file included from include/linux/wait.h:9:
In file included from include/linux/spinlock.h:56:
In file included from include/linux/preempt.h:79:
In file included from arch/x86/include/asm/preempt.h:9:
In file included from include/linux/thread_info.h:60:
In file included from arch/x86/include/asm/thread_info.h:53:
In file included from arch/x86/include/asm/cpufeature.h:5:
In file included from arch/x86/include/asm/processor.h:19:
In file included from arch/x86/include/asm/cpuid.h:62:
In file included from arch/x86/include/asm/paravirt.h:17:
In file included from include/linux/cpumask.h:12:
In file included from include/linux/bitmap.h:12:
In file included from include/linux/string.h:295:
include/linux/fortify-string.h:489:4: error: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror,-Wattribute-warning]
  489 |                         __write_overflow_field(p_size_field, size);
      |                         ^
1 error generated.

cvise spits out:

char calc_ntlmv2_hash_ses_1;
long calc_ntlmv2_hash___trans_tmp_3, calc_ntlmv2_hash___trans_tmp_1;
short *calc_ntlmv2_hash_user;
void __write_overflow_field()
    __attribute__((__warning__("detected write beyond size of field (1st "
                               "parameter); maybe use struct_group()?")));
__attribute__((__alloc_size__(1))) void *kmalloc(long);
int calc_ntlmv2_hash() {
  int len = calc_ntlmv2_hash_ses_1
                ? __builtin_choose_expr(0, 0, calc_ntlmv2_hash___trans_tmp_3)
                : 0;
  calc_ntlmv2_hash_user = kmalloc(2 + len * 2);
  if (len)
    ;
  else {
    long __fortify_size = 2, size = __fortify_size;
    calc_ntlmv2_hash___trans_tmp_1 =
        __builtin_dynamic_object_size(calc_ntlmv2_hash_user, 1);
    int p_size_field = calc_ntlmv2_hash___trans_tmp_1;
    if (p_size_field < size)
      __write_overflow_field();
  }
  return 0;
}

@ llvm/llvm-project@a1b9736:

$ clang -O2 -c -o /dev/null cifsencrypt.i

@ llvm/llvm-project@d77067d:

$ clang -O2 -c -o /dev/null cifsencrypt.i
cifsencrypt.i:21:7: warning: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
   21 |       __write_overflow_field();
      |       ^
1 warning generated.

GCC 13.2.0 also does not show any warning.

[nathanchance]

Whoops, I forgot to mention this is originally from the memset(user, '\0', 2); in calc_ntlmv2_hash(), as this diff hides the warning:

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index ef4c2e3c9fa6..9c4c532e49a7 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -571,8 +571,10 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
        if (len) {
                len = cifs_strtoUTF16(user, ses->user_name, len, nls_cp);
                UniStrupr(user);
+#if 0
        } else {
                memset(user, '\0', 2);
+#endif
        }

        rc = crypto_shash_update(ses->server->secmech.hmacmd5,

[nathanchance]

I spent some more time looking into this today. A semi-coherent brain dump follows :P

That example may have been overreduced, this one was done more manually but I can verify that GCC and Clang 17.0.1 do not warn whereas current trunk does.

typedef unsigned short __u16;
typedef __u16 __le16;
typedef unsigned long size_t;

struct cifs_ses {
    char *user_name;
};

struct shash_desc;

__attribute__((__alloc_size__(1))) __attribute__((__malloc__)) void *kmalloc(size_t size);

int crypto_shash_update(const unsigned char *data, unsigned int len);

void fortify_panic(const char *name) __attribute__((__noreturn__)) __attribute__((__cold__));
void __write_overflow(void) __attribute__((__error__("detected write beyond size of object (1st parameter)")));
void __write_overflow_field(size_t avail, size_t wanted) __attribute__((__warning__("detected write beyond size of field (1st parameter); maybe use struct_group()?")));

extern inline __attribute__((__gnu_inline__)) __attribute__((__unused__)) __attribute__((no_instrument_function)) __attribute__((__always_inline__)) __attribute__((__gnu_inline__)) __attribute__((__overloadable__)) void fortify_memset_chk(size_t size,
      const size_t p_size,
      const size_t p_size_field)
{
 if (__builtin_constant_p(size)) {
  if (( __builtin_constant_p((p_size_field) < (p_size)) && (p_size_field) < (p_size) ) &&
      ( __builtin_constant_p((p_size) < (size)) && (p_size) < (size) ))
   __write_overflow();

  if (( __builtin_constant_p((p_size_field) < (size)) && (p_size_field) < (size) ))
   __write_overflow_field(p_size_field, size);
 }
 if (p_size != (~(size_t)0) && p_size < size)
  fortify_panic("memset");
}

extern size_t __real_strnlen(const char *, size_t) __asm__("strnlen");
extern inline __attribute__((__gnu_inline__)) __attribute__((__unused__)) __attribute__((no_instrument_function)) __attribute__((__always_inline__)) __attribute__((__gnu_inline__)) __attribute__((__overloadable__)) size_t strnlen(const char * const __attribute__
((__pass_dynamic_object_size__(1))) p, 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__((__unused__)) __attribute__((no_instrument_function)) __attribute__((__always_inline__)) __attribute__((__gnu_inline__)) __attribute__((__overloadable__)) __attribute__((__diagnose_as_builtin__(__builtin_strlen,
 1)))
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);
 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;
}

int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash)
{
    __le16 *user;
    int len;

    len = ses->user_name ? __builtin_choose_expr((sizeof(int) == sizeof(*(8 ? ((void *)((long)(__builtin_strlen(ses->user_name)) * 0l)) : (int *)8))), __builtin_strlen(ses->user_name), __fortify_strlen(ses->user_name)) : 0;
    user = kmalloc(2 + (len * 2));
    if (!user)
        return -1;
    
    if (len)
        ;
    else {
        ({ size_t __fortify_size = (size_t)(2); fortify_memset_chk(__fortify_size, __builtin_dynamic_object_size(user, 0), __builtin_dynamic_object_size(user, 1)), __builtin_memset(user, '\0', __fortify_size); });
    }

    return crypto_shash_update((unsigned char *)user, len * 2);
}

https://godbolt.org/z/bMe9b1cer

The diff of the IR is somewhat interesting (you can see it on Godbolt by adding -emit-llvm to the clang ones), as it appears that it has added __write_overflow_field() to the basic block that contains fortify_panic():

Diff of LLVM IR

diff --git a/tmp/17.0.1 b/tmp/trunk
index f69abc0..1668835 100644
--- a/tmp/17.0.1
+++ b/tmp/trunk
@@ -6,7 +6,7 @@ define dso_local i32 @calc_ntlmv2_hash(ptr nocapture noundef readonly %0, ptr no
   br i1 %4, label %8, label %5
 
 5:
-  %6 = tail call i64 @strlen(ptr noundef nonnull dereferenceable(1) %3) #5
+  %6 = tail call i64 @strlen(ptr noundef nonnull dereferenceable(1) %3) #6
   %7 = trunc i64 %6 to i32
   br label %8
 
@@ -15,7 +15,7 @@ define dso_local i32 @calc_ntlmv2_hash(ptr nocapture noundef readonly %0, ptr no
   %10 = shl nsw i32 %9, 1
   %11 = add nsw i32 %10, 2
   %12 = sext i32 %11 to i64
-  %13 = tail call noalias ptr @kmalloc(i64 noundef %12) #6
+  %13 = tail call noalias ptr @kmalloc(i64 noundef %12) #7
   %14 = icmp eq ptr %13, null
   br i1 %14, label %23, label %15
 
@@ -28,7 +28,8 @@ define dso_local i32 @calc_ntlmv2_hash(ptr nocapture noundef readonly %0, ptr no
   br i1 %18, label %19, label %20
 
 19:
-  tail call void @fortify_panic(ptr noundef nonnull @.str) #7
+  tail call void @__write_overflow_field(i64 noundef %12, i64 noundef 2) #6
+  tail call void @fortify_panic(ptr noundef nonnull @.str) #8
   unreachable
 
 20:
@@ -36,7 +37,7 @@ define dso_local i32 @calc_ntlmv2_hash(ptr nocapture noundef readonly %0, ptr no
   br label %21
 
 21:
-  %22 = tail call i32 @crypto_shash_update(ptr noundef nonnull %13, i32 noundef %10) #5
+  %22 = tail call i32 @crypto_shash_update(ptr noundef nonnull %13, i32 noundef %10) #6
   br label %23
 
 23:
@@ -52,11 +53,14 @@ declare i64 @strlen(ptr nocapture noundef) local_unnamed_addr #3
 
 declare void @fortify_panic(ptr noundef) local_unnamed_addr #4
 
+declare void @__write_overflow_field(i64 noundef, i64 noundef) local_unnamed_addr #5
+
 attributes #0 = { nounwind uwtable "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
 attributes #1 = { allocsize(0) "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
 attributes #2 = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
 attributes #3 = { mustprogress nofree nounwind willreturn memory(argmem: read) "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
 attributes #4 = { cold noreturn "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
-attributes #5 = { nounwind }
-attributes #6 = { nounwind allocsize(0) }
-attributes #7 = { cold noreturn nounwind }
+attributes #5 = { "dontcall-warn"="detected write beyond size of field (1st parameter); maybe use struct_group()?" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
+attributes #6 = { nounwind }
+attributes #7 = { nounwind allocsize(0) }
+attributes #8 = { cold noreturn nounwind }

This ultimately seems related to the fact that strlen() returns size_t (ultimately typedef'd to unsigned long), as it is being assigned to len which is type int. The warning goes away with this diff:

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index ef4c2e3c9fa6..fff88b1fa5aa 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -533,7 +533,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
                            const struct nls_table *nls_cp)
 {
        int rc = 0;
-       int len;
+       size_t len;
        char nt_hash[CIFS_NTHASH_SIZE];
        __le16 *user;
        wchar_t *domain;

I am not sure what llvm/llvm-project@d77067d has to do with that. I suspect it has something to do with the nsw keyword on shl and add with the type mismatch, which appears to allows LLVM to combine the compile time and the run time checks?

That diff is probably the right solution for this because of the type mismatch as it is but many of the functions that use len expect a type int so we'd just be punting the issue further down the road but maybe that's okay?

I may be missing something else obvious here too.

cc @kees, I had pinged about this on IRC but I am not sure how active you have been on there especially with the holidays.

[kees]

Hmm. I haven't dug into the IR or anything, but I'm always suspicious of these kinds of tiny memsets. This also makes the warning go away, by not using memset. :P

diff --git a/fs/smb/client/cifsencrypt.c b/fs/smb/client/cifsencrypt.c
index ef4c2e3c9fa6..0a0f0e1d43be 100644
--- a/fs/smb/client/cifsencrypt.c
+++ b/fs/smb/client/cifsencrypt.c
@@ -572,7 +572,7 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
                len = cifs_strtoUTF16(user, ses->user_name, len, nls_cp);
                UniStrupr(user);
        } else {
-               memset(user, '\0', 2);
+               *user = 0;
        }
 
        rc = crypto_shash_update(ses->server->secmech.hmacmd5,

[kees]

It does seem like a Clang bug, though, since len really shouldn't have any affect on the memset. Perhaps we need to open a bug with LLVM and make sure to CC Nikita Popov?

[nathanchance]

since len really shouldn't have any affect on the memset

Is this true though? The size of user depends on len, which is used for __builtin_dynamic_object_size(), right (since kmalloc() has __alloc_size(1))?

I have a standalone reproducer that can be run in user space, which never triggers fortify_panic() for me, so this definitely seems like a false positive. I can file an issue upstream later today if this seems like a reasonable test case.

repro.c

#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>

typedef unsigned long	__kernel_ulong_t;
typedef __kernel_ulong_t __kernel_size_t;
typedef __kernel_size_t		size_t;

#define SIZE_MAX	(~(size_t)0)

#define __cold					__attribute__((__cold__))
#define __compiletime_error(msg)		__attribute__((__error__(msg)))
#define __compiletime_warning(msg)		__attribute__((__warning__(msg)))
#define __diagnose_as(builtin...)		__attribute__((__diagnose_as_builtin__(builtin)))
#define __pass_dynamic_object_size(type)	__attribute__((__pass_dynamic_object_size__(type)))
#define __gnu_inline				__attribute__((__gnu_inline__))
#define __noreturn				__attribute__((__noreturn__))
#define __overloadable				__attribute__((__overloadable__))

#define __FORTIFY_INLINE extern __always_inline __gnu_inline __overloadable
#define __RENAME(x) __asm__(#x)

#define __struct_size(p)	__builtin_dynamic_object_size(p, 0)
#define __member_size(p)	__builtin_dynamic_object_size(p, 1)
#define POS			__pass_dynamic_object_size(1)
#define POS0			__pass_dynamic_object_size(0)

void fortify_panic(const char *name) __noreturn __cold;
void __write_overflow(void) __compiletime_error("detected write beyond size of object (1st parameter)");
void __write_overflow_field(size_t avail, size_t wanted) __compiletime_warning("detected write beyond size of field (1st parameter); maybe use struct_group()?");

#define __compiletime_lessthan(bounds, length)	(	\
	__builtin_constant_p((bounds) < (length)) &&	\
	(bounds) < (length)				\
)

#define __compiletime_strlen(p)					\
({								\
	char *__p = (char *)(p);				\
	size_t __ret = SIZE_MAX;				\
	const size_t __p_size = __member_size(p);		\
	if (__p_size != SIZE_MAX &&				\
	    __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;							\
})

#define __is_constexpr(x) \
	(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))

#define strlen(p)							\
	__builtin_choose_expr(__is_constexpr(__builtin_strlen(p)),	\
		__builtin_strlen(p), __fortify_strlen(p))

#define __underlying_memset	__builtin_memset
#define __underlying_strlen	__builtin_strlen

extern __kernel_size_t __real_strnlen(const char *, __kernel_size_t) __RENAME(strnlen);
/**
 * strnlen - Return bounded count of characters in a NUL-terminated string
 *
 * @p: pointer to NUL-terminated string to count.
 * @maxlen: maximum number of characters to count.
 *
 * Returns number of characters in @p (NOT including the final NUL), or
 * @maxlen, if no NUL has been found up to there.
 *
 */
__FORTIFY_INLINE __kernel_size_t strnlen(const char * const POS p, __kernel_size_t maxlen)
{
	const size_t p_size = __member_size(p);
	const size_t p_len = __compiletime_strlen(p);
	size_t ret;

	/* We can take compile-time actions when maxlen is const. */
	if (__builtin_constant_p(maxlen) && p_len != SIZE_MAX) {
		/* If p is const, we can use its compile-time-known len. */
		if (maxlen >= p_size)
			return p_len;
	}

	/* Do not check characters beyond the end of p. */
	ret = __real_strnlen(p, maxlen < p_size ? maxlen : p_size);
	if (p_size <= ret && maxlen != ret)
		fortify_panic(__func__);
	return ret;
}

__FORTIFY_INLINE __diagnose_as(__builtin_strlen, 1)
__kernel_size_t __fortify_strlen(const char * const POS p)
{
	const size_t p_size = __member_size(p);
	__kernel_size_t ret;

	/* Give up if we don't know how large p is. */
	if (p_size == SIZE_MAX)
		return __underlying_strlen(p);
	ret = strnlen(p, p_size);
	if (p_size <= ret)
		fortify_panic(__func__);
	return ret;
}

__FORTIFY_INLINE void fortify_memset_chk(__kernel_size_t size,
					 const size_t p_size,
					 const size_t p_size_field)
{
	if (__builtin_constant_p(size)) {
		/*
		 * Length argument is a constant expression, so we
		 * can perform compile-time bounds checking where
		 * buffer sizes are also known at compile time.
		 */

		/* Error when size is larger than enclosing struct. */
		if (__compiletime_lessthan(p_size_field, p_size) &&
		    __compiletime_lessthan(p_size, size))
			__write_overflow();

		/* Warn when write size is larger than dest field. */
		if (__compiletime_lessthan(p_size_field, size))
			__write_overflow_field(p_size_field, size);
	}
	/*
	 * At this point, length argument may not be a constant expression,
	 * so run-time bounds checking can be done where buffer sizes are
	 * known. (This is not an "else" because the above checks may only
	 * be compile-time warnings, and we want to still warn for run-time
	 * overflows.)
	 */

	/*
	 * Always stop accesses beyond the struct that contains the
	 * field, when the buffer's remaining size is known.
	 * (The SIZE_MAX test is to optimize away checks where the buffer
	 * lengths are unknown.)
	 */
	if (p_size != SIZE_MAX && p_size < size)
		fortify_panic("memset");
}

#define __fortify_memset_chk(p, c, size, p_size, p_size_field) ({	\
	size_t __fortify_size = (size_t)(size);				\
	fortify_memset_chk(__fortify_size, p_size, p_size_field),	\
	__underlying_memset(p, c, __fortify_size);			\
})

#define memset(p, c, s) __fortify_memset_chk(p, c, s,			\
		__struct_size(p), __member_size(p))

#define show(exp) printf(#exp ": %zu\n", exp)

extern int crypto_shash_update(const unsigned char *buffer, unsigned int len);

int main(int argc, char **argv)
{
	unsigned short *user;
	char *user_name = NULL;
	int len;

	if (argc > 1)
		user_name = argv[1];

	len = user_name ? strlen(user_name) : 0;
	user = malloc(2 + (len * 2));
	if (!user)
		return -1;

	show(__struct_size(user));
	show(__member_size(user));

	if (len)
		printf("Length of user_name is not zero\n");
	else
		memset(user, '\0', 2);

	return crypto_shash_update((unsigned char *)user, len * 2);
}

lib.c

#include <stddef.h>
#include <stdio.h>
#include <stdlib.h>

void fortify_panic(const char *name)
{
	fprintf(stderr, "detected buffer overflow in %s\n", name);
	exit(1);
}

size_t strnlen(const char *s, size_t count)
{
	const char *sc;

	for (sc = s; count-- && *sc != '\0'; ++sc)
		/* nothing */;
	return sc - s;
}

int crypto_shash_update(const unsigned char *buffer, unsigned int len)
{
	return len % 2;
}

void __write_overflow_field(size_t avail, size_t wanted)
{
}

$ clang -O2 repro.c lib.c
repro.c:126:4: warning: call to '__write_overflow_field' declared with 'warning' attribute: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
  126 |                         __write_overflow_field(p_size_field, size);
      |                         ^
1 warning generated.

$ ./a.out adsf
__struct_size(user): 10
__member_size(user): 10
Length of user_name is not zero

$ ./a.out
__struct_size(user): 2
__member_size(user): 2

[kees]

since len really shouldn't have any affect on the memset

Is this true though? The size of user depends on len, which is used for __builtin_dynamic_object_size(), right (since kmalloc() has __alloc_size(1))?

Oh I see what you mean -- len determines the __alloc_size hinting that __bdos ultimately depends on. Yeah, very good point. Yes, please open a bug for this -- something very weird is happening. Should I send the memset removal patch in the meantime to make this warning go away? (You've got the repro so we can test any LLVM fixes...)

[nathanchance]

Upstream issue filed: llvm/llvm-project#77813

Should I send the memset removal patch in the meantime to make this warning go away? (You've got the repro so we can test any LLVM fixes...)

I think we can wait to see what upstream says/recommends. This is only visible with tip of tree LLVM at the moment, so I don't necessarily think we need to rush a fix.

[nathanchance]

I have not seen any movement on the upstream report and this is starting to get annoying in CI due to CONFIG_WERROR :/ should we just send the workaround now since it seems uncontroversial on the surface?

[kees]

Sent:
https://lore.kernel.org/linux-hardening/20240123234731.work.358-kees@kernel.org/

[nathanchance]

https://git.kernel.org/linus/8deb05c84b63b4fdb8549e08942867a68924a5b8