Re: startup BUG at lib/string_helpers.c from scsi fusion mptsas

[Kees Cook <keescook@chromium.org>]

On Mon, Apr 08, 2024 at 12:59:52PM -0700, Justin Stitt wrote:
> https://lore.kernel.org/all/d45631ac-3ee6-45cc-8b5a-fab130ce39d7@cox.net/
> 
> On Sat, Apr 6, 2024 at 1:42 PM Charles Bertsch <cbertsch@cox.net> wrote:
> > Justin,
> > Yes, undo of that patch does fix the problem, the scsi controller and
> > all disks are visible.
> >
> > So did changing .config so that FORTIFY would not be used.
> >
> > Given other messages on this subject, there seems a basic conflict
> > between using strscpy() to mean -- copy however much will fit, and leave
> > a proper NUL-terminated string, versus FORTIFY trying to signal that
> > something has been lost. Is there a strscpy variation (_pad maybe?) that
> > FORTIFY will remain calm about truncation?
> 
> I think fortified strscpy() should allow for the truncation, this, at
> least in my eyes, is the expected behavior of strscpy(). You copy as
> much as you can from the source and slap a '\0' to the end without
> overflowing the destination.

The trouble is with truncation. Some strings:

char longstr[]  = "This is long."; // sizeof(longstr) == 14, strlen(longstr) == 13
char truncstr[] = "This is trunc";
char nonstr[sizeof(truncstr) - 1]; // sizeof(nonstr) == 13
char dest[64];

/* Create "nonstr" without a trailing NUL */
memcpy(nonstr, trunc, strlen(trunc));

strscpy(dest, long, sizeof(dest) /* 64 */)
	=> 13 (i.e. strlen(longstr))
strscpy(dest, long, strlen(longstr) + 1 /* 14 */)
	=> 13
strscpy(dest, long, strlen(longstr) /* 13 */)
	=> -E2BIG, we saw that "." wasn't a NUL
strscpy(dest, nonstr, 13)
	=> -E2BIG, we saw that "." wasn't a NUL
strscpy(dest, nonstr, 14)
	=> fortify error, we can't examine the char after "."

If we used sizeof(src) to try to work around the off-by-one, we'd
suddenly lose the ability to detect actually corrupt strings, because
we'd silently start "accepting" strings that were exactly sized off by
1, and gain ambiguity in our handling.

> I think Kees has some plans to address this as we spoke offline.

But, this use of strncpy() *is* another "legitimate" use-case. But like
the other strncpy() uses, it is ambiguous. So, I think we need the
reverse of strtomem(), to take a "maybe NUL padded but not terminated"
character array and unambiguously construct a NUL-terminated string from
it.

I think something like this, memtostr:

diff --git a/include/linux/string.h b/include/linux/string.h
index 9ba8b4597009..5def02c7c0ce 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -422,6 +422,30 @@ void memcpy_and_pad(void *dest, size_t dest_len, const void *src, size_t count,
 	memcpy(dest, src, strnlen(src, min(_src_len, _dest_len)));	\
 } while (0)
 
+/**
+ * memtostr - Copy a possibly non-NUL-term string to a NUL-term string
+ * @dest: Pointer to destination NUL-terminates string
+ * @src: Pointer to character array (likely marked as __nonstring)
+ *
+ * This is a replacement for strncpy() uses where the source is not
+ * a NUL-terminated string.
+ *
+ * Note that sizes of @dest and @src must be known at compile-time.
+ */
+#define memtostr(dest, src)	do {					\
+	const size_t _dest_len = __builtin_object_size(dest, 1);	\
+	const size_t _src_len = __builtin_object_size(src, 1);		\
+	const size_t _src_chars = strnlen(src, _src_len);		\
+	const size_t _copy_len = min(_dest_len - 1, _src_chars);	\
+									\
+	BUILD_BUG_ON(!__builtin_constant_p(_dest_len) ||		\
+		     !__builtin_constant_p(_src_len) ||			\
+		     _dest_len == 0 || _dest_len == (size_t)-1 ||	\
+		     _src_len == 0 || _src_len == (size_t)-1);		\
+	memcpy(dest, src, _copy_len);					\
+	dest[_copy_len] = '\0';						\
+} while (0)
+
 /**
  * memset_after - Set a value after a struct member to the end of a struct
  *


I've also identified other cases where this pattern exists, so I think
we can apply this and any needed fixes using it instead of strscpy().

-- 
Kees Cook