| Message ID | 20240410023155.2100422-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: Avoid possible run-time warning with long manufacturer strings | expand |
On Wed, Apr 10, 2024 at 5:31 AM Kees Cook <keescook@chromium.org> wrote: > > Another ambiguous use of strncpy() is to copy from strings that may not > be NUL-terminated. These cases depend on having the destination buffer > be explicitly larger than the source buffer's maximum size, having > the size of the copy exactly match the source buffer's maximum size, > and for the destination buffer to get explicitly NUL terminated. > > This usually happens when parsing protocols or hardware character arrays > that are not guaranteed to be NUL-terminated. The code pattern is > effectively this: > > char dest[sizeof(src) + 1]; > > strncpy(dest, src, sizeof(src)); > dest[sizeof(dest) - 1] = '\0'; > > In practice it usually looks like: > > struct from_hardware { > ... > char name[HW_NAME_SIZE] __nonstring; > ... > }; > > struct from_hardware *p = ...; > char name[HW_NAME_SIZE + 1]; > > strncpy(name, p->name, HW_NAME_SIZE); > name[NW_NAME_SIZE] = '\0'; > > This cannot be replaced with: > > strscpy(name, p->name, sizeof(name)); > > because p->name is smaller and not NUL-terminated, so FORTIFY will > trigger when strnlen(p->name, sizeof(name)) is used. And it cannot be > replaced with: > > strscpy(name, p->name, sizeof(p->name)); > > because then "name" may contain a 1 character early truncation of > p->name. > > Provide an unambiguous interface for converting a maybe not-NUL-terminated > string to a NUL-terminated string, with compile-time buffer size checking > so that it can never fail at runtime: memtostr() and memtostr_pad(). Also > add KUnit tests for both. Obvious question, why can't strscpy() be fixed for this corner case?
On Wed, Apr 10, 2024 at 07:08:10AM +0300, Andy Shevchenko wrote: > On Wed, Apr 10, 2024 at 5:31 AM Kees Cook <keescook@chromium.org> wrote: > > > > Another ambiguous use of strncpy() is to copy from strings that may not > > be NUL-terminated. These cases depend on having the destination buffer > > be explicitly larger than the source buffer's maximum size, having > > the size of the copy exactly match the source buffer's maximum size, > > and for the destination buffer to get explicitly NUL terminated. > > > > This usually happens when parsing protocols or hardware character arrays > > that are not guaranteed to be NUL-terminated. The code pattern is > > effectively this: > > > > char dest[sizeof(src) + 1]; > > > > strncpy(dest, src, sizeof(src)); > > dest[sizeof(dest) - 1] = '\0'; > > > > In practice it usually looks like: > > > > struct from_hardware { > > ... > > char name[HW_NAME_SIZE] __nonstring; > > ... > > }; > > > > struct from_hardware *p = ...; > > char name[HW_NAME_SIZE + 1]; > > > > strncpy(name, p->name, HW_NAME_SIZE); > > name[NW_NAME_SIZE] = '\0'; > > > > This cannot be replaced with: > > > > strscpy(name, p->name, sizeof(name)); > > > > because p->name is smaller and not NUL-terminated, so FORTIFY will > > trigger when strnlen(p->name, sizeof(name)) is used. And it cannot be > > replaced with: > > > > strscpy(name, p->name, sizeof(p->name)); > > > > because then "name" may contain a 1 character early truncation of > > p->name. > > > > Provide an unambiguous interface for converting a maybe not-NUL-terminated > > string to a NUL-terminated string, with compile-time buffer size checking > > so that it can never fail at runtime: memtostr() and memtostr_pad(). Also > > add KUnit tests for both. > > Obvious question, why can't strscpy() be fixed for this corner case? We would lose the ability to detect normal out-of-bounds reads, or at least make them ambiguous. I really want these APIs to have distinct and dependable semantics/behaviors.
On Tue, Apr 09, 2024 at 07:31:50PM -0700, Kees Cook wrote: > Another ambiguous use of strncpy() is to copy from strings that may not > be NUL-terminated. These cases depend on having the destination buffer > be explicitly larger than the source buffer's maximum size, having > the size of the copy exactly match the source buffer's maximum size, > and for the destination buffer to get explicitly NUL terminated. > > This usually happens when parsing protocols or hardware character arrays > that are not guaranteed to be NUL-terminated. The code pattern is > effectively this: > > char dest[sizeof(src) + 1]; > > strncpy(dest, src, sizeof(src)); > dest[sizeof(dest) - 1] = '\0'; > > In practice it usually looks like: > > struct from_hardware { > ... > char name[HW_NAME_SIZE] __nonstring; > ... > }; > > struct from_hardware *p = ...; > char name[HW_NAME_SIZE + 1]; > > strncpy(name, p->name, HW_NAME_SIZE); > name[NW_NAME_SIZE] = '\0'; > > This cannot be replaced with: > > strscpy(name, p->name, sizeof(name)); > > because p->name is smaller and not NUL-terminated, so FORTIFY will > trigger when strnlen(p->name, sizeof(name)) is used. And it cannot be > replaced with: > > strscpy(name, p->name, sizeof(p->name)); > > because then "name" may contain a 1 character early truncation of > p->name. > > Provide an unambiguous interface for converting a maybe not-NUL-terminated > string to a NUL-terminated string, with compile-time buffer size checking > so that it can never fail at runtime: memtostr() and memtostr_pad(). Also > add KUnit tests for both. > > Signed-off-by: Kees Cook <keescook@chromium.org> FYI, As the string KUnit tests have seen some refactoring, I'm taking this patch and refactoring it onto my tree. Once the SCSI fixes are reviewed, if we want to land them in -next, it's probably easiest for them to go via my tree. -Kees
Kees, > As the string KUnit tests have seen some refactoring, I'm taking this > patch and refactoring it onto my tree. Once the SCSI fixes are > reviewed, if we want to land them in -next, it's probably easiest for > them to go via my tree. Sure, no problem.
diff --git a/include/linux/string.h b/include/linux/string.h index 793c27ad7c0d..bd42cf85a95b 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -424,6 +424,55 @@ 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) + +/** + * memtostr_pad - Copy a possibly non-NUL-term string to a NUL-term string + * with NUL padding in the destination + * @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_pad(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); \ + memset(&dest[_copy_len], 0, _dest_len - _copy_len); \ +} while (0) + /** * memset_after - Set a value after a struct member to the end of a struct * diff --git a/lib/strscpy_kunit.c b/lib/strscpy_kunit.c index a6b6344354ed..ac0b5d1678b3 100644 --- a/lib/strscpy_kunit.c +++ b/lib/strscpy_kunit.c @@ -126,8 +126,34 @@ static void strscpy_test(struct kunit *test) KUNIT_EXPECT_EQ(test, strscpy(dest, "This is too long", ARRAY_SIZE(dest)), -E2BIG); } +static void memtostr_test(struct kunit *test) +{ + char nonstring[7] = { 'a', 'b', 'c', 'd', 'e', 'f', 'g' }; + char nonstring_small[3] = { 'a', 'b', 'c' }; + char dest[sizeof(nonstring) + 1]; + + /* Copy in a non-NUL-terminated string into exactly right-sized dest. */ + KUNIT_EXPECT_EQ(test, sizeof(dest), sizeof(nonstring) + 1); + memset(dest, 'X', sizeof(dest)); + memtostr(dest, nonstring); + KUNIT_EXPECT_STREQ(test, dest, "abcdefg"); + memset(dest, 'X', sizeof(dest)); + memtostr(dest, nonstring_small); + KUNIT_EXPECT_STREQ(test, dest, "abc"); + KUNIT_EXPECT_EQ(test, dest[7], 'X'); + + memset(dest, 'X', sizeof(dest)); + memtostr_pad(dest, nonstring); + KUNIT_EXPECT_STREQ(test, dest, "abcdefg"); + memset(dest, 'X', sizeof(dest)); + memtostr_pad(dest, nonstring_small); + KUNIT_EXPECT_STREQ(test, dest, "abc"); + KUNIT_EXPECT_EQ(test, dest[7], '\0'); +} + static struct kunit_case strscpy_test_cases[] = { KUNIT_CASE(strscpy_test), + KUNIT_CASE(memtostr_test), {} };
Another ambiguous use of strncpy() is to copy from strings that may not be NUL-terminated. These cases depend on having the destination buffer be explicitly larger than the source buffer's maximum size, having the size of the copy exactly match the source buffer's maximum size, and for the destination buffer to get explicitly NUL terminated. This usually happens when parsing protocols or hardware character arrays that are not guaranteed to be NUL-terminated. The code pattern is effectively this: char dest[sizeof(src) + 1]; strncpy(dest, src, sizeof(src)); dest[sizeof(dest) - 1] = '\0'; In practice it usually looks like: struct from_hardware { ... char name[HW_NAME_SIZE] __nonstring; ... }; struct from_hardware *p = ...; char name[HW_NAME_SIZE + 1]; strncpy(name, p->name, HW_NAME_SIZE); name[NW_NAME_SIZE] = '\0'; This cannot be replaced with: strscpy(name, p->name, sizeof(name)); because p->name is smaller and not NUL-terminated, so FORTIFY will trigger when strnlen(p->name, sizeof(name)) is used. And it cannot be replaced with: strscpy(name, p->name, sizeof(p->name)); because then "name" may contain a 1 character early truncation of p->name. Provide an unambiguous interface for converting a maybe not-NUL-terminated string to a NUL-terminated string, with compile-time buffer size checking so that it can never fail at runtime: memtostr() and memtostr_pad(). Also add KUnit tests for both. Signed-off-by: Kees Cook <keescook@chromium.org> --- Cc: Justin Stitt <justinstitt@google.com> Cc: Andy Shevchenko <andy@kernel.org> Cc: linux-hardening@vger.kernel.org --- include/linux/string.h | 49 ++++++++++++++++++++++++++++++++++++++++++ lib/strscpy_kunit.c | 26 ++++++++++++++++++++++ 2 files changed, 75 insertions(+)