| Message ID | 20231012-strncpy-drivers-net-ethernet-qlogic-qlcnic-qlcnic_83xx_init-c-v1-1-f0008d5e43be@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | f8dd2412ba66128de4325c187c9504c6da511bc8 |
| Headers | show |
| Series | qlcnic: replace deprecated strncpy with strscpy | expand |
On Thu, Oct 12, 2023 at 07:44:29PM +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 fw_info->fw_file_name to be NUL-terminated based on its use > within _request_firmware_prepare() wherein `name` refers to it: > | if (firmware_request_builtin_buf(firmware, name, dbuf, size)) { > | dev_dbg(device, "using built-in %s\n", name); > | return 0; /* assigned */ > | } > ... and with firmware_request_builtin() also via `name`: > | if (strcmp(name, b_fw->name) == 0) { > > There is no evidence that NUL-padding is required. > > 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. When doing the hard-coded value to sizeof(), can you include in the commit log the rationale for it? For example: Additionally replace size macro (QLC_FW_FILE_NAME_LEN) with sizeof(fw_info->fw_file_name) to more directly tie the maximum buffer size to the destination buffer: struct qlc_83xx_fw_info { ... char fw_file_name[QLC_FW_FILE_NAME_LEN]; }; > > 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/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c > index c95d56e56c59..b733374b4dc5 100644 > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c > @@ -2092,8 +2092,8 @@ static int qlcnic_83xx_run_post(struct qlcnic_adapter *adapter) > return -EINVAL; > } > > - strncpy(fw_info->fw_file_name, QLC_83XX_POST_FW_FILE_NAME, > - QLC_FW_FILE_NAME_LEN); > + strscpy(fw_info->fw_file_name, QLC_83XX_POST_FW_FILE_NAME, > + sizeof(fw_info->fw_file_name)); > > ret = request_firmware(&fw_info->fw, fw_info->fw_file_name, dev); > if (ret) { > @@ -2396,12 +2396,12 @@ static int qlcnic_83xx_get_fw_info(struct qlcnic_adapter *adapter) > switch (pdev->device) { > case PCI_DEVICE_ID_QLOGIC_QLE834X: > case PCI_DEVICE_ID_QLOGIC_QLE8830: > - strncpy(fw_info->fw_file_name, QLC_83XX_FW_FILE_NAME, > - QLC_FW_FILE_NAME_LEN); > + strscpy(fw_info->fw_file_name, QLC_83XX_FW_FILE_NAME, > + sizeof(fw_info->fw_file_name)); > break; > case PCI_DEVICE_ID_QLOGIC_QLE844X: > - strncpy(fw_info->fw_file_name, QLC_84XX_FW_FILE_NAME, > - QLC_FW_FILE_NAME_LEN); > + strscpy(fw_info->fw_file_name, QLC_84XX_FW_FILE_NAME, > + sizeof(fw_info->fw_file_name)); > break; > default: > dev_err(&pdev->dev, "%s: Invalid device id\n", But yes, this all looks right to me. Reviewed-by: Kees Cook <keescook@chromium.org>
On Sat, Oct 14, 2023 at 7:47 PM Kees Cook <keescook@chromium.org> wrote: > > On Thu, Oct 12, 2023 at 07:44:29PM +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 fw_info->fw_file_name to be NUL-terminated based on its use > > within _request_firmware_prepare() wherein `name` refers to it: > > | if (firmware_request_builtin_buf(firmware, name, dbuf, size)) { > > | dev_dbg(device, "using built-in %s\n", name); > > | return 0; /* assigned */ > > | } > > ... and with firmware_request_builtin() also via `name`: > > | if (strcmp(name, b_fw->name) == 0) { > > > > There is no evidence that NUL-padding is required. > > > > 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. > > When doing the hard-coded value to sizeof(), can you include in the > commit log the rationale for it? For example: > > Additionally replace size macro (QLC_FW_FILE_NAME_LEN) with > sizeof(fw_info->fw_file_name) to more directly tie the maximum buffer > size to the destination buffer: > > struct qlc_83xx_fw_info { > ... > char fw_file_name[QLC_FW_FILE_NAME_LEN]; > }; > Sure, I'll be doing this from now on to make the change more clear! > > > > > 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/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c | 12 ++++++------ > > 1 file changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c > > index c95d56e56c59..b733374b4dc5 100644 > > --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c > > +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c > > @@ -2092,8 +2092,8 @@ static int qlcnic_83xx_run_post(struct qlcnic_adapter *adapter) > > return -EINVAL; > > } > > > > - strncpy(fw_info->fw_file_name, QLC_83XX_POST_FW_FILE_NAME, > > - QLC_FW_FILE_NAME_LEN); > > + strscpy(fw_info->fw_file_name, QLC_83XX_POST_FW_FILE_NAME, > > + sizeof(fw_info->fw_file_name)); > > > > ret = request_firmware(&fw_info->fw, fw_info->fw_file_name, dev); > > if (ret) { > > @@ -2396,12 +2396,12 @@ static int qlcnic_83xx_get_fw_info(struct qlcnic_adapter *adapter) > > switch (pdev->device) { > > case PCI_DEVICE_ID_QLOGIC_QLE834X: > > case PCI_DEVICE_ID_QLOGIC_QLE8830: > > - strncpy(fw_info->fw_file_name, QLC_83XX_FW_FILE_NAME, > > - QLC_FW_FILE_NAME_LEN); > > + strscpy(fw_info->fw_file_name, QLC_83XX_FW_FILE_NAME, > > + sizeof(fw_info->fw_file_name)); > > break; > > case PCI_DEVICE_ID_QLOGIC_QLE844X: > > - strncpy(fw_info->fw_file_name, QLC_84XX_FW_FILE_NAME, > > - QLC_FW_FILE_NAME_LEN); > > + strscpy(fw_info->fw_file_name, QLC_84XX_FW_FILE_NAME, > > + sizeof(fw_info->fw_file_name)); > > break; > > default: > > dev_err(&pdev->dev, "%s: Invalid device id\n", > > But yes, this all looks right to me. > > Reviewed-by: Kees Cook <keescook@chromium.org> > > -- > Kees Cook Thanks Justin
On Thu, 12 Oct 2023 19:44:29 +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 fw_info->fw_file_name to be NUL-terminated based on its use > within _request_firmware_prepare() wherein `name` refers to it: > | if (firmware_request_builtin_buf(firmware, name, dbuf, size)) { > | dev_dbg(device, "using built-in %s\n", name); > | return 0; /* assigned */ > | } > ... and with firmware_request_builtin() also via `name`: > | if (strcmp(name, b_fw->name) == 0) { > > [...] Applied to for-next/hardening, thanks! [1/1] qlcnic: replace deprecated strncpy with strscpy https://git.kernel.org/kees/c/f8bef1ef8095 Take care,
On Thu, 30 Nov 2023 14:00:28 -0800 Kees Cook wrote: > [1/1] qlcnic: replace deprecated strncpy with strscpy > https://git.kernel.org/kees/c/f8bef1ef8095 You asked for changes yourself here, please drop all the networking patches you applied today :|
On Thu, Nov 30, 2023 at 10:43:12PM -0800, Jakub Kicinski wrote: > On Thu, 30 Nov 2023 14:00:28 -0800 Kees Cook wrote: > > [1/1] qlcnic: replace deprecated strncpy with strscpy > > https://git.kernel.org/kees/c/f8bef1ef8095 > > You asked for changes yourself here, please drop all the networking > patches you applied today :| It was a request for future commit logs. But yes, dropped. Justin, can you refresh this patch as well, and include the comments about the sizeof() replacement being safe?
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c index c95d56e56c59..b733374b4dc5 100644 --- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c +++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c @@ -2092,8 +2092,8 @@ static int qlcnic_83xx_run_post(struct qlcnic_adapter *adapter) return -EINVAL; } - strncpy(fw_info->fw_file_name, QLC_83XX_POST_FW_FILE_NAME, - QLC_FW_FILE_NAME_LEN); + strscpy(fw_info->fw_file_name, QLC_83XX_POST_FW_FILE_NAME, + sizeof(fw_info->fw_file_name)); ret = request_firmware(&fw_info->fw, fw_info->fw_file_name, dev); if (ret) { @@ -2396,12 +2396,12 @@ static int qlcnic_83xx_get_fw_info(struct qlcnic_adapter *adapter) switch (pdev->device) { case PCI_DEVICE_ID_QLOGIC_QLE834X: case PCI_DEVICE_ID_QLOGIC_QLE8830: - strncpy(fw_info->fw_file_name, QLC_83XX_FW_FILE_NAME, - QLC_FW_FILE_NAME_LEN); + strscpy(fw_info->fw_file_name, QLC_83XX_FW_FILE_NAME, + sizeof(fw_info->fw_file_name)); break; case PCI_DEVICE_ID_QLOGIC_QLE844X: - strncpy(fw_info->fw_file_name, QLC_84XX_FW_FILE_NAME, - QLC_FW_FILE_NAME_LEN); + strscpy(fw_info->fw_file_name, QLC_84XX_FW_FILE_NAME, + sizeof(fw_info->fw_file_name)); break; default: dev_err(&pdev->dev, "%s: Invalid device id\n",
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 fw_info->fw_file_name to be NUL-terminated based on its use within _request_firmware_prepare() wherein `name` refers to it: | if (firmware_request_builtin_buf(firmware, name, dbuf, size)) { | dev_dbg(device, "using built-in %s\n", name); | return 0; /* assigned */ | } ... and with firmware_request_builtin() also via `name`: | if (strcmp(name, b_fw->name) == 0) { There is no evidence that NUL-padding is required. 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. 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/net/ethernet/qlogic/qlcnic/qlcnic_83xx_init.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) --- base-commit: cbf3a2cb156a2c911d8f38d8247814b4c07f49a2 change-id: 20231012-strncpy-drivers-net-ethernet-qlogic-qlcnic-qlcnic_83xx_init-c-7858019dc583 Best regards, -- Justin Stitt <justinstitt@google.com>