On Mon, Apr 08, 2024 at 12:59:52PM -0700, Justin Stitt wrote: > https://lore.kernel.org/all/d45631ac-3ee6-45cc-8b5a-fab130ce39d7@xxxxxxx/ > > On Sat, Apr 6, 2024 at 1:42 PM Charles Bertsch <cbertsch@xxxxxxx> 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
