* [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
@ 2024-04-10 2:31 Kees Cook
2024-04-10 2:31 ` [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad() Kees Cook
` (6 more replies)
0 siblings, 7 replies; 14+ messages in thread
From: Kees Cook @ 2024-04-10 2:31 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Kees Cook, Charles Bertsch, Justin Stitt, Bart Van Assche,
Andy Shevchenko, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, James E.J. Bottomley, Kashyap Desai,
Sumit Saxena, Nilesh Javali, Andrew Morton, Himanshu Madhani,
linux-kernel, linux-hardening, MPT-FusionLinux.pdl, linux-scsi,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
Hi,
Another code pattern using the gloriously ambiguous strncpy() function was
turning maybe not-NUL-terminated character arrays into NUL-terminated
strings. In these cases, when strncpy() is replaced with strscpy()
it creates a situation where if the non-terminated string takes up the
entire character array (i.e. it is not terminated) run-time warnings
from CONFIG_FORTIFY_SOURCE will trip, since strscpy() was expecting to
find a NUL-terminated source but checking for the NUL would walk off
the end of the source buffer.
In doing an instrumented build of the kernel to find these cases, it
seems it was almost entirely a code pattern used in the SCSI subsystem,
so the creation of the new helper, memtostr(), can land via the SCSI
tree. And, as it turns out, inappropriate conversions have been happening
for several years now, some of which even moved through strlcpy() first
(and were never noticed either).
This series fixes all 4 of the instances I could find in the SCSI
subsystem.
Thanks,
-Kees
Kees Cook (5):
string.h: Introduce memtostr() and memtostr_pad()
scsi: mptfusion: Avoid possible run-time warning with long
manufacturer strings
scsi: mpt3sas: Avoid possible run-time warning with long manufacturer
strings
scsi: mpi3mr: Avoid possible run-time warning with long manufacturer
strings
scsi: qla2xxx: Avoid possible run-time warning with long model_num
drivers/message/fusion/mptsas.c | 14 +++----
drivers/scsi/mpi3mr/mpi3mr_transport.c | 14 +++----
drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++----
drivers/scsi/qla2xxx/qla_mr.c | 6 +--
include/linux/string.h | 49 ++++++++++++++++++++++++
lib/strscpy_kunit.c | 26 +++++++++++++
7 files changed, 93 insertions(+), 32 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad()
2024-04-10 2:31 [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
@ 2024-04-10 2:31 ` Kees Cook
2024-04-10 4:08 ` Andy Shevchenko
2024-04-24 15:59 ` Kees Cook
2024-04-10 2:31 ` [PATCH 2/5] scsi: mptfusion: Avoid possible run-time warning with long manufacturer strings Kees Cook
` (5 subsequent siblings)
6 siblings, 2 replies; 14+ messages in thread
From: Kees Cook @ 2024-04-10 2:31 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Kees Cook, Justin Stitt, Andy Shevchenko, linux-hardening,
Charles Bertsch, Bart Van Assche, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, James E.J. Bottomley, Kashyap Desai,
Sumit Saxena, Nilesh Javali, Andrew Morton, Himanshu Madhani,
linux-kernel, MPT-FusionLinux.pdl, linux-scsi,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
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(+)
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),
{}
};
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/5] scsi: mptfusion: Avoid possible run-time warning with long manufacturer strings
2024-04-10 2:31 [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
2024-04-10 2:31 ` [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad() Kees Cook
@ 2024-04-10 2:31 ` Kees Cook
2024-04-10 2:31 ` [PATCH 3/5] scsi: mpt3sas: " Kees Cook
` (4 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2024-04-10 2:31 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Kees Cook, Charles Bertsch, Justin Stitt, Sathya Prakash,
Sreekanth Reddy, Suganath Prabu Subramani, MPT-FusionLinux.pdl,
linux-scsi, Bart Van Assche, Andy Shevchenko,
James E.J. Bottomley, Kashyap Desai, Sumit Saxena, Nilesh Javali,
Andrew Morton, Himanshu Madhani, linux-kernel, linux-hardening,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
The prior strscpy() replacement of strncpy() here expected the
manufacture_reply strings to be NUL-terminated, but it is possible
they are not, as the code pattern here shows, e.g., edev->vendor_id
being exactly 1 character larger than manufacture_reply->vendor_id,
and the replaced strncpy() was copying only up to the size of the
source character array. Replace this with memtostr(), which is the
unambiguous way to convert a maybe not-NUL-terminated character array
into a NUL-terminated string.
Reported-by: Charles Bertsch <cbertsch@cox.net>
Closes: https://lore.kernel.org/all/5445ba0f-3e27-4d43-a9ba-0cc22ada2fce@cox.net/
Fixes: 45e833f0e5bb ("scsi: message: fusion: Replace deprecated strncpy() with strscpy()")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Justin Stitt <justinstitt@google.com>
Cc: Sathya Prakash <sathya.prakash@broadcom.com>
Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
Cc: MPT-FusionLinux.pdl@broadcom.com
Cc: linux-scsi@vger.kernel.org
---
drivers/message/fusion/mptsas.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 300f8e955a53..0f80c840afc3 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -2964,17 +2964,13 @@ mptsas_exp_repmanufacture_info(MPT_ADAPTER *ioc,
goto out_free;
manufacture_reply = data_out + sizeof(struct rep_manu_request);
- strscpy(edev->vendor_id, manufacture_reply->vendor_id,
- sizeof(edev->vendor_id));
- strscpy(edev->product_id, manufacture_reply->product_id,
- sizeof(edev->product_id));
- strscpy(edev->product_rev, manufacture_reply->product_rev,
- sizeof(edev->product_rev));
+ memtostr(edev->vendor_id, manufacture_reply->vendor_id);
+ memtostr(edev->product_id, manufacture_reply->product_id);
+ memtostr(edev->product_rev, manufacture_reply->product_rev);
edev->level = manufacture_reply->sas_format;
if (manufacture_reply->sas_format) {
- strscpy(edev->component_vendor_id,
- manufacture_reply->component_vendor_id,
- sizeof(edev->component_vendor_id));
+ memtostr(edev->component_vendor_id,
+ manufacture_reply->component_vendor_id);
tmp = (u8 *)&manufacture_reply->component_id;
edev->component_id = tmp[0] << 8 | tmp[1];
edev->component_revision_id =
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/5] scsi: mpt3sas: Avoid possible run-time warning with long manufacturer strings
2024-04-10 2:31 [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
2024-04-10 2:31 ` [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad() Kees Cook
2024-04-10 2:31 ` [PATCH 2/5] scsi: mptfusion: Avoid possible run-time warning with long manufacturer strings Kees Cook
@ 2024-04-10 2:31 ` Kees Cook
2024-04-10 2:31 ` [PATCH 4/5] scsi: mpi3mr: " Kees Cook
` (3 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2024-04-10 2:31 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Kees Cook, Justin Stitt, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, James E.J. Bottomley,
MPT-FusionLinux.pdl, linux-scsi, Charles Bertsch, Bart Van Assche,
Andy Shevchenko, Kashyap Desai, Sumit Saxena, Nilesh Javali,
Andrew Morton, Himanshu Madhani, linux-kernel, linux-hardening,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
The prior strscpy() replacement of strncpy() here expected the
manufacture_reply strings to be NUL-terminated, but it is possible
they are not, as the code pattern here shows, e.g., edev->vendor_id
being exactly 1 character larger than manufacture_reply->vendor_id,
and the replaced strncpy() was copying only up to the size of the
source character array. Replace this with memtostr(), which is the
unambiguous way to convert a maybe not-NUL-terminated character array
into a NUL-terminated string.
Fixes: b7e9712a02e8 ("scsi: mpt3sas: Replace deprecated strncpy() with strscpy()")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Justin Stitt <justinstitt@google.com>
Cc: Sathya Prakash <sathya.prakash@broadcom.com>
Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
Cc: Suganath Prabu Subramani <suganath-prabu.subramani@broadcom.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: MPT-FusionLinux.pdl@broadcom.com
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++++---------
2 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 258647fc6bdd..1320e06727df 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -4774,7 +4774,7 @@ _base_display_ioc_capabilities(struct MPT3SAS_ADAPTER *ioc)
char desc[17] = {0};
u32 iounit_pg1_flags;
- strscpy(desc, ioc->manu_pg0.ChipName, sizeof(desc));
+ memtostr(desc, ioc->manu_pg0.ChipName);
ioc_info(ioc, "%s: FWVersion(%02d.%02d.%02d.%02d), ChipRevision(0x%02x)\n",
desc,
(ioc->facts.FWVersion.Word & 0xFF000000) >> 24,
diff --git a/drivers/scsi/mpt3sas/mpt3sas_transport.c b/drivers/scsi/mpt3sas/mpt3sas_transport.c
index 76f9a9177198..d84413b77d84 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_transport.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_transport.c
@@ -458,17 +458,13 @@ _transport_expander_report_manufacture(struct MPT3SAS_ADAPTER *ioc,
goto out;
manufacture_reply = data_out + sizeof(struct rep_manu_request);
- strscpy(edev->vendor_id, manufacture_reply->vendor_id,
- sizeof(edev->vendor_id));
- strscpy(edev->product_id, manufacture_reply->product_id,
- sizeof(edev->product_id));
- strscpy(edev->product_rev, manufacture_reply->product_rev,
- sizeof(edev->product_rev));
+ memtostr(edev->vendor_id, manufacture_reply->vendor_id);
+ memtostr(edev->product_id, manufacture_reply->product_id);
+ memtostr(edev->product_rev, manufacture_reply->product_rev);
edev->level = manufacture_reply->sas_format & 1;
if (edev->level) {
- strscpy(edev->component_vendor_id,
- manufacture_reply->component_vendor_id,
- sizeof(edev->component_vendor_id));
+ memtostr(edev->component_vendor_id,
+ manufacture_reply->component_vendor_id);
tmp = (u8 *)&manufacture_reply->component_id;
edev->component_id = tmp[0] << 8 | tmp[1];
edev->component_revision_id =
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/5] scsi: mpi3mr: Avoid possible run-time warning with long manufacturer strings
2024-04-10 2:31 [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
` (2 preceding siblings ...)
2024-04-10 2:31 ` [PATCH 3/5] scsi: mpt3sas: " Kees Cook
@ 2024-04-10 2:31 ` Kees Cook
2024-04-10 2:31 ` [PATCH 5/5] scsi: qla2xxx: Avoid possible run-time warning with long model_num Kees Cook
` (2 subsequent siblings)
6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2024-04-10 2:31 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Kees Cook, Sreekanth Reddy, Sathya Prakash Veerichetty,
Kashyap Desai, Sumit Saxena, James E.J. Bottomley,
mpi3mr-linuxdrv.pdl, linux-scsi, Charles Bertsch, Justin Stitt,
Bart Van Assche, Andy Shevchenko, Suganath Prabu Subramani,
Nilesh Javali, Andrew Morton, Himanshu Madhani, linux-kernel,
linux-hardening, MPT-FusionLinux.pdl, GR-QLogic-Storage-Upstream
The prior use of strscpy() here expected the manufacture_reply strings to
be NUL-terminated, but it is possible they are not, as the code pattern
here shows, e.g., edev->vendor_id being exactly 1 character larger than
manufacture_reply->vendor_id, and the strscpy() was copying only up to
the size of the source character array. Replace this with memtostr(),
which is the unambiguous way to convert a maybe not-NUL-terminated
character array into a NUL-terminated string.
Fixes: 2bd37e284914 ("scsi: mpi3mr: Add framework to issue MPT transport cmds")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Sreekanth Reddy <sreekanth.reddy@broadcom.com>
Cc: Sathya Prakash Veerichetty <sathya.prakash@broadcom.com>
Cc: Kashyap Desai <kashyap.desai@broadcom.com>
Cc: Sumit Saxena <sumit.saxena@broadcom.com>
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: mpi3mr-linuxdrv.pdl@broadcom.com
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/mpi3mr/mpi3mr_transport.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index dabb91f0f75d..3caceddb864a 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -211,17 +211,13 @@ static int mpi3mr_report_manufacture(struct mpi3mr_ioc *mrioc,
goto out;
}
- strscpy(edev->vendor_id, manufacture_reply->vendor_id,
- SAS_EXPANDER_VENDOR_ID_LEN);
- strscpy(edev->product_id, manufacture_reply->product_id,
- SAS_EXPANDER_PRODUCT_ID_LEN);
- strscpy(edev->product_rev, manufacture_reply->product_rev,
- SAS_EXPANDER_PRODUCT_REV_LEN);
+ memtostr(edev->vendor_id, manufacture_reply->vendor_id);
+ memtostr(edev->product_id, manufacture_reply->product_id);
+ memtostr(edev->product_rev, manufacture_reply->product_rev);
edev->level = manufacture_reply->sas_format & 1;
if (edev->level) {
- strscpy(edev->component_vendor_id,
- manufacture_reply->component_vendor_id,
- SAS_EXPANDER_COMPONENT_VENDOR_ID_LEN);
+ memtostr(edev->component_vendor_id,
+ manufacture_reply->component_vendor_id);
tmp = (u8 *)&manufacture_reply->component_id;
edev->component_id = tmp[0] << 8 | tmp[1];
edev->component_revision_id =
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/5] scsi: qla2xxx: Avoid possible run-time warning with long model_num
2024-04-10 2:31 [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
` (3 preceding siblings ...)
2024-04-10 2:31 ` [PATCH 4/5] scsi: mpi3mr: " Kees Cook
@ 2024-04-10 2:31 ` Kees Cook
2024-04-17 17:35 ` [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
2024-04-25 1:55 ` Martin K. Petersen
6 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2024-04-10 2:31 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Kees Cook, Bart Van Assche, Nilesh Javali,
GR-QLogic-Storage-Upstream, James E.J. Bottomley, linux-scsi,
Charles Bertsch, Justin Stitt, Andy Shevchenko, Sathya Prakash,
Sreekanth Reddy, Suganath Prabu Subramani, Kashyap Desai,
Sumit Saxena, Andrew Morton, Himanshu Madhani, linux-kernel,
linux-hardening, MPT-FusionLinux.pdl, mpi3mr-linuxdrv.pdl
The prior strlcpy() replacement of strncpy() here (which was
later replaced with strscpy()) expected pinfo->model_num (and
pinfo->model_description) to be NUL-terminated, but it is possible
it was not, as the code pattern here shows vha->hw->model_number (and
vha->hw->model_desc) being exactly 1 character larger, and the replaced
strncpy() was copying only up to the size of the source character
array. Replace this with memtostr(), which is the unambiguous way to
convert a maybe not-NUL-terminated character array into a NUL-terminated
string.
Fixes: 527e9b704c3d ("scsi: qla2xxx: Use memcpy() and strlcpy() instead of strcpy() and strncpy()")
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Nilesh Javali <njavali@marvell.com>
Cc: GR-QLogic-Storage-Upstream@marvell.com
Cc: "James E.J. Bottomley" <jejb@linux.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/qla2xxx/qla_mr.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c
index 083f94e43fba..82a7e21ddc83 100644
--- a/drivers/scsi/qla2xxx/qla_mr.c
+++ b/drivers/scsi/qla2xxx/qla_mr.c
@@ -1909,10 +1909,8 @@ qlafx00_fx_disc(scsi_qla_host_t *vha, fc_port_t *fcport, uint16_t fx_type)
if (fx_type == FXDISC_GET_CONFIG_INFO) {
struct config_info_data *pinfo =
(struct config_info_data *) fdisc->u.fxiocb.rsp_addr;
- strscpy(vha->hw->model_number, pinfo->model_num,
- ARRAY_SIZE(vha->hw->model_number));
- strscpy(vha->hw->model_desc, pinfo->model_description,
- ARRAY_SIZE(vha->hw->model_desc));
+ memtostr(vha->hw->model_number, pinfo->model_num);
+ memtostr(vha->hw->model_desc, pinfo->model_description);
memcpy(&vha->hw->mr.symbolic_name, pinfo->symbolic_name,
sizeof(vha->hw->mr.symbolic_name));
memcpy(&vha->hw->mr.serial_num, pinfo->serial_num,
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad()
2024-04-10 2:31 ` [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad() Kees Cook
@ 2024-04-10 4:08 ` Andy Shevchenko
2024-04-10 18:33 ` Kees Cook
2024-04-24 15:59 ` Kees Cook
1 sibling, 1 reply; 14+ messages in thread
From: Andy Shevchenko @ 2024-04-10 4:08 UTC (permalink / raw)
To: Kees Cook
Cc: Martin K . Petersen, Justin Stitt, Andy Shevchenko,
linux-hardening, Charles Bertsch, Bart Van Assche, Sathya Prakash,
Sreekanth Reddy, Suganath Prabu Subramani, James E.J. Bottomley,
Kashyap Desai, Sumit Saxena, Nilesh Javali, Andrew Morton,
Himanshu Madhani, linux-kernel, MPT-FusionLinux.pdl, linux-scsi,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
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?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad()
2024-04-10 4:08 ` Andy Shevchenko
@ 2024-04-10 18:33 ` Kees Cook
0 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2024-04-10 18:33 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Martin K . Petersen, Justin Stitt, Andy Shevchenko,
linux-hardening, Charles Bertsch, Bart Van Assche, Sathya Prakash,
Sreekanth Reddy, Suganath Prabu Subramani, James E.J. Bottomley,
Kashyap Desai, Sumit Saxena, Nilesh Javali, Andrew Morton,
Himanshu Madhani, linux-kernel, MPT-FusionLinux.pdl, linux-scsi,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
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.
--
Kees Cook
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
2024-04-10 2:31 [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
` (4 preceding siblings ...)
2024-04-10 2:31 ` [PATCH 5/5] scsi: qla2xxx: Avoid possible run-time warning with long model_num Kees Cook
@ 2024-04-17 17:35 ` Kees Cook
2024-04-18 0:35 ` Martin K. Petersen
2024-04-25 1:55 ` Martin K. Petersen
6 siblings, 1 reply; 14+ messages in thread
From: Kees Cook @ 2024-04-17 17:35 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Charles Bertsch, Justin Stitt, Bart Van Assche, Andy Shevchenko,
Sathya Prakash, Sreekanth Reddy, Suganath Prabu Subramani,
James E.J. Bottomley, Kashyap Desai, Sumit Saxena, Nilesh Javali,
Andrew Morton, Himanshu Madhani, linux-kernel, linux-hardening,
MPT-FusionLinux.pdl, linux-scsi, mpi3mr-linuxdrv.pdl,
GR-QLogic-Storage-Upstream
On Tue, Apr 09, 2024 at 07:31:49PM -0700, Kees Cook wrote:
> Hi,
>
> Another code pattern using the gloriously ambiguous strncpy() function was
> turning maybe not-NUL-terminated character arrays into NUL-terminated
> strings. In these cases, when strncpy() is replaced with strscpy()
> it creates a situation where if the non-terminated string takes up the
> entire character array (i.e. it is not terminated) run-time warnings
> from CONFIG_FORTIFY_SOURCE will trip, since strscpy() was expecting to
> find a NUL-terminated source but checking for the NUL would walk off
> the end of the source buffer.
>
> In doing an instrumented build of the kernel to find these cases, it
> seems it was almost entirely a code pattern used in the SCSI subsystem,
> so the creation of the new helper, memtostr(), can land via the SCSI
> tree. And, as it turns out, inappropriate conversions have been happening
> for several years now, some of which even moved through strlcpy() first
> (and were never noticed either).
>
> This series fixes all 4 of the instances I could find in the SCSI
> subsystem.
Friendly ping. Can the SCSI tree pick this up, or should I take it
through the hardening tree?
Thanks!
-Kees
>
> Thanks,
>
> -Kees
>
> Kees Cook (5):
> string.h: Introduce memtostr() and memtostr_pad()
> scsi: mptfusion: Avoid possible run-time warning with long
> manufacturer strings
> scsi: mpt3sas: Avoid possible run-time warning with long manufacturer
> strings
> scsi: mpi3mr: Avoid possible run-time warning with long manufacturer
> strings
> scsi: qla2xxx: Avoid possible run-time warning with long model_num
>
> drivers/message/fusion/mptsas.c | 14 +++----
> drivers/scsi/mpi3mr/mpi3mr_transport.c | 14 +++----
> drivers/scsi/mpt3sas/mpt3sas_base.c | 2 +-
> drivers/scsi/mpt3sas/mpt3sas_transport.c | 14 +++----
> drivers/scsi/qla2xxx/qla_mr.c | 6 +--
> include/linux/string.h | 49 ++++++++++++++++++++++++
> lib/strscpy_kunit.c | 26 +++++++++++++
> 7 files changed, 93 insertions(+), 32 deletions(-)
>
> --
> 2.34.1
>
--
Kees Cook
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
2024-04-17 17:35 ` [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
@ 2024-04-18 0:35 ` Martin K. Petersen
2024-04-18 17:40 ` Kees Cook
0 siblings, 1 reply; 14+ messages in thread
From: Martin K. Petersen @ 2024-04-18 0:35 UTC (permalink / raw)
To: Kees Cook
Cc: Martin K . Petersen, Charles Bertsch, Justin Stitt,
Bart Van Assche, Andy Shevchenko, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, James E.J. Bottomley, Kashyap Desai,
Sumit Saxena, Nilesh Javali, Andrew Morton, Himanshu Madhani,
linux-kernel, linux-hardening, MPT-FusionLinux.pdl, linux-scsi,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
Hi Kees!
>> This series fixes all 4 of the instances I could find in the SCSI
>> subsystem.
>
> Friendly ping. Can the SCSI tree pick this up, or should I take it
> through the hardening tree?
It's on my list of series to review. Have a couple of fires going right
now.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
2024-04-18 0:35 ` Martin K. Petersen
@ 2024-04-18 17:40 ` Kees Cook
0 siblings, 0 replies; 14+ messages in thread
From: Kees Cook @ 2024-04-18 17:40 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Charles Bertsch, Justin Stitt, Bart Van Assche, Andy Shevchenko,
Sathya Prakash, Sreekanth Reddy, Suganath Prabu Subramani,
James E.J. Bottomley, Kashyap Desai, Sumit Saxena, Nilesh Javali,
Andrew Morton, Himanshu Madhani, linux-kernel, linux-hardening,
MPT-FusionLinux.pdl, linux-scsi, mpi3mr-linuxdrv.pdl,
GR-QLogic-Storage-Upstream
On Wed, Apr 17, 2024 at 08:35:15PM -0400, Martin K. Petersen wrote:
>
> Hi Kees!
>
> >> This series fixes all 4 of the instances I could find in the SCSI
> >> subsystem.
> >
> > Friendly ping. Can the SCSI tree pick this up, or should I take it
> > through the hardening tree?
>
> It's on my list of series to review. Have a couple of fires going right
> now.
Okay, thanks! I just wanted to make sure it got picked up for v6.9 since
it fixes a real-world issue. :)
--
Kees Cook
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad()
2024-04-10 2:31 ` [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad() Kees Cook
2024-04-10 4:08 ` Andy Shevchenko
@ 2024-04-24 15:59 ` Kees Cook
2024-04-25 1:42 ` Martin K. Petersen
1 sibling, 1 reply; 14+ messages in thread
From: Kees Cook @ 2024-04-24 15:59 UTC (permalink / raw)
To: Martin K . Petersen
Cc: Justin Stitt, Andy Shevchenko, linux-hardening, Charles Bertsch,
Bart Van Assche, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, James E.J. Bottomley, Kashyap Desai,
Sumit Saxena, Nilesh Javali, Andrew Morton, Himanshu Madhani,
linux-kernel, MPT-FusionLinux.pdl, linux-scsi,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
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 Cook
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad()
2024-04-24 15:59 ` Kees Cook
@ 2024-04-25 1:42 ` Martin K. Petersen
0 siblings, 0 replies; 14+ messages in thread
From: Martin K. Petersen @ 2024-04-25 1:42 UTC (permalink / raw)
To: Kees Cook
Cc: Martin K . Petersen, Justin Stitt, Andy Shevchenko,
linux-hardening, Charles Bertsch, Bart Van Assche, Sathya Prakash,
Sreekanth Reddy, Suganath Prabu Subramani, James E.J. Bottomley,
Kashyap Desai, Sumit Saxena, Nilesh Javali, Andrew Morton,
Himanshu Madhani, linux-kernel, MPT-FusionLinux.pdl, linux-scsi,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
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.
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings
2024-04-10 2:31 [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
` (5 preceding siblings ...)
2024-04-17 17:35 ` [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
@ 2024-04-25 1:55 ` Martin K. Petersen
6 siblings, 0 replies; 14+ messages in thread
From: Martin K. Petersen @ 2024-04-25 1:55 UTC (permalink / raw)
To: Kees Cook
Cc: Martin K . Petersen, Charles Bertsch, Justin Stitt,
Bart Van Assche, Andy Shevchenko, Sathya Prakash, Sreekanth Reddy,
Suganath Prabu Subramani, James E.J. Bottomley, Kashyap Desai,
Sumit Saxena, Nilesh Javali, Andrew Morton, Himanshu Madhani,
linux-kernel, linux-hardening, MPT-FusionLinux.pdl, linux-scsi,
mpi3mr-linuxdrv.pdl, GR-QLogic-Storage-Upstream
Kees,
> This series fixes all 4 of the instances I could find in the SCSI
> subsystem.
Looks OK to me.
Minor nit: I do find it a bit odd to think of a string as "memory".
Maybe that's just because I am so used to always having to distinguish
between fixed length strings and NUL-terminated strings in the storage
protocols and hardware programming interfaces. But both types are
definitely referred to as "strings" colloquially and not so much
"memory".
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-04-25 1:55 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-10 2:31 [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
2024-04-10 2:31 ` [PATCH 1/5] string.h: Introduce memtostr() and memtostr_pad() Kees Cook
2024-04-10 4:08 ` Andy Shevchenko
2024-04-10 18:33 ` Kees Cook
2024-04-24 15:59 ` Kees Cook
2024-04-25 1:42 ` Martin K. Petersen
2024-04-10 2:31 ` [PATCH 2/5] scsi: mptfusion: Avoid possible run-time warning with long manufacturer strings Kees Cook
2024-04-10 2:31 ` [PATCH 3/5] scsi: mpt3sas: " Kees Cook
2024-04-10 2:31 ` [PATCH 4/5] scsi: mpi3mr: " Kees Cook
2024-04-10 2:31 ` [PATCH 5/5] scsi: qla2xxx: Avoid possible run-time warning with long model_num Kees Cook
2024-04-17 17:35 ` [PATCH 0/5] scsi: Avoid possible run-time warning with long manufacturer strings Kees Cook
2024-04-18 0:35 ` Martin K. Petersen
2024-04-18 17:40 ` Kees Cook
2024-04-25 1:55 ` Martin K. Petersen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).