| Message ID | 20231023-strncpy-drivers-scsi-3w-sas-c-v1-1-4c40a1e99dfc@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 7936a19e944b934d21d79f1b90d478d1f7081b63 |
| Headers | show |
| Series | scsi: 3w-sas: replace deprecated strncpy with strscpy | expand |
On Mon, Oct 23, 2023 at 07:50:57PM +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. > > This pattern of strncpy(dest, src, strlen(src)) is extremely bug-prone. > This pattern basically never results in NUL-terminated destination > strings unless `dest` was zero-initialized. The current implementation > may be accidentally correct as tw_dev is zero-allocated via: > > host = scsi_host_alloc(&driver_template, sizeof(TW_Device_Extension)); > ... > tw_dev = shost_priv(host); > > ... wherein scsi_host_alloc zero-allocates host: > > shost = kzalloc(sizeof(struct Scsi_Host) + privsize, GFP_KERNEL); > > Also, further suggesting this change is worthwhile is another strscpy() > usage in 32-9xxx.c: > > strscpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, > sizeof(tw_dev->tw_compat_info.driver_version)); > > Considering the above, a suitable replacement is `strscpy` [2] due to > the fact that it guarantees NUL-termination on the destination buffer > without unnecessarily NUL-padding. > > Let's not be accidentally correct, let's be definitely correct. > > 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> Looks legit. 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.8/scsi-staging, thanks!
On Mon, 23 Oct 2023 19:50:57 +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. > > This pattern of strncpy(dest, src, strlen(src)) is extremely bug-prone. > This pattern basically never results in NUL-terminated destination > strings unless `dest` was zero-initialized. The current implementation > may be accidentally correct as tw_dev is zero-allocated via: > > [...] Applied to 6.8/scsi-queue, thanks! [1/1] scsi: 3w-sas: replace deprecated strncpy with strscpy https://git.kernel.org/mkp/scsi/c/7936a19e944b
diff --git a/drivers/scsi/3w-sas.c b/drivers/scsi/3w-sas.c index 55989eaa2d9f..9bdb75dfdcd7 100644 --- a/drivers/scsi/3w-sas.c +++ b/drivers/scsi/3w-sas.c @@ -1326,7 +1326,8 @@ static int twl_reset_sequence(TW_Device_Extension *tw_dev, int soft_reset) } /* Load rest of compatibility struct */ - strncpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, strlen(TW_DRIVER_VERSION)); + strscpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, + sizeof(tw_dev->tw_compat_info.driver_version)); tw_dev->tw_compat_info.driver_srl_high = TW_CURRENT_DRIVER_SRL; tw_dev->tw_compat_info.driver_branch_high = TW_CURRENT_DRIVER_BRANCH; tw_dev->tw_compat_info.driver_build_high = TW_CURRENT_DRIVER_BUILD;
strncpy() is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. This pattern of strncpy(dest, src, strlen(src)) is extremely bug-prone. This pattern basically never results in NUL-terminated destination strings unless `dest` was zero-initialized. The current implementation may be accidentally correct as tw_dev is zero-allocated via: host = scsi_host_alloc(&driver_template, sizeof(TW_Device_Extension)); ... tw_dev = shost_priv(host); ... wherein scsi_host_alloc zero-allocates host: shost = kzalloc(sizeof(struct Scsi_Host) + privsize, GFP_KERNEL); Also, further suggesting this change is worthwhile is another strscpy() usage in 32-9xxx.c: strscpy(tw_dev->tw_compat_info.driver_version, TW_DRIVER_VERSION, sizeof(tw_dev->tw_compat_info.driver_version)); Considering the above, a suitable replacement is `strscpy` [2] due to the fact that it guarantees NUL-termination on the destination buffer without unnecessarily NUL-padding. Let's not be accidentally correct, let's be definitely correct. 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> --- Note: build-tested only. Found with: $ rg "strncpy\(" --- drivers/scsi/3w-sas.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) --- base-commit: 9c5d00cb7b6bbc5a7965d9ab7d223b5402d1f02c change-id: 20231023-strncpy-drivers-scsi-3w-sas-c-c7cc037d4bc9 Best regards, -- Justin Stitt <justinstitt@google.com>