| Message ID | 20240226-strncpy-drivers-scsi-lpfc-lpfc_ct-c-v2-1-2df2e46569b9@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | e100c01efa85c8a0ee7527bf28ef7ea7c3ca57e1 |
| Headers | show |
| Series | [v2] scsi: lpfc: replace deprecated strncpy with strscpy | expand |
On Mon, Feb 26, 2024 at 11:53:44PM +0000, Justin Stitt wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect ae->value_string to be NUL-terminated because there's a > comment that says as much; these attr strings are also used with other > string APIs, further cementing the fact. > > Now, the question of whether or not to NUL-pad the destination buffer: > lpfc_fdmi_rprt_defer() initializes vports (all zero-initialized), then > we call lpfc_fdmi_cmd() with each vport and a mask. Then, inside of > lpfc_fdmi_cmd() we check each bit in the mask to invoke the proper > callback. Importantly, the zero-initialized vport is passed in as the > "attr" parameter. Seeing this: > | struct lpfc_fdmi_attr_string *ae = attr; > ... we can tell that ae->value_string is entirely zero-initialized. Due > to this, NUL-padding is _not_ required as it would be redundant. > > Considering the above, a suitable replacement is `strscpy` [2]. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> Reviewed-by: Kees Cook <keescook@chromium.org>
Justin, > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. Applied to 6.9/scsi-staging, thanks!
On Mon, 26 Feb 2024 23:53:44 +0000, Justin Stitt wrote: > strncpy() is deprecated for use on NUL-terminated destination strings > [1] and as such we should prefer more robust and less ambiguous string > interfaces. > > We expect ae->value_string to be NUL-terminated because there's a > comment that says as much; these attr strings are also used with other > string APIs, further cementing the fact. > > [...] Applied to 6.9/scsi-queue, thanks! [1/1] scsi: lpfc: replace deprecated strncpy with strscpy https://git.kernel.org/mkp/scsi/c/e100c01efa85
diff --git a/drivers/scsi/lpfc/lpfc_ct.c b/drivers/scsi/lpfc/lpfc_ct.c index baae1f8279e0..296d2d4796cb 100644 --- a/drivers/scsi/lpfc/lpfc_ct.c +++ b/drivers/scsi/lpfc/lpfc_ct.c @@ -2569,9 +2569,9 @@ lpfc_fdmi_set_attr_string(void *attr, uint16_t attrtype, char *attrstring) * 64 bytes or less. */ - strncpy(ae->value_string, attrstring, sizeof(ae->value_string)); + strscpy(ae->value_string, attrstring, sizeof(ae->value_string)); len = strnlen(ae->value_string, sizeof(ae->value_string)); - /* round string length to a 32bit boundary. Ensure there's a NULL */ + /* round string length to a 32bit boundary */ len += (len & 3) ? (4 - (len & 3)) : 4; /* size is Type/Len (4 bytes) plus string length */ size = FOURBYTES + len;
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. We expect ae->value_string to be NUL-terminated because there's a comment that says as much; these attr strings are also used with other string APIs, further cementing the fact. Now, the question of whether or not to NUL-pad the destination buffer: lpfc_fdmi_rprt_defer() initializes vports (all zero-initialized), then we call lpfc_fdmi_cmd() with each vport and a mask. Then, inside of lpfc_fdmi_cmd() we check each bit in the mask to invoke the proper callback. Importantly, the zero-initialized vport is passed in as the "attr" parameter. Seeing this: | struct lpfc_fdmi_attr_string *ae = attr; ... we can tell that ae->value_string is entirely zero-initialized. Due to this, NUL-padding is _not_ required as it would be redundant. Considering the above, a suitable replacement is `strscpy` [2]. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://manpages.debian.org/testing/linux-manual-4.8/strscpy.9.en.html [2] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Changes in v2: - keep strnlen (thanks Kees) - Link to v1: https://lore.kernel.org/all/20240222-strncpy-drivers-scsi-lpfc-lpfc_ct-c-v1-1-20c685bd1b43@google.com/ --- drivers/scsi/lpfc/lpfc_ct.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- base-commit: 39133352cbed6626956d38ed72012f49b0421e7b change-id: 20240222-strncpy-drivers-scsi-lpfc-lpfc_ct-c-f54b67eeeb68 Best regards, -- Justin Stitt <justinstitt@google.com>