On Wed, Oct 18, 2023 at 10:48:49PM +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 both data->subsysnqn and data->hostnqn to be NUL-terminated
> based on their usage with format specifier ("%s"):
> fabrics.c:
> 322: dev_err(ctrl->device,
> 323: "%s, subsysnqn \"%s\"\n",
> 324: inv_data, data->subsysnqn);
> ...
> 349: dev_err(ctrl->device,
> 350: "Connect for subsystem %s is not allowed, hostnqn: %s\n",
> 351: data->subsysnqn, data->hostnqn);
>
> Moreover, there's no need to NUL-pad since `data` is zero-allocated
> already in fabrics.c:
> 383: data = kzalloc(sizeof(*data), GFP_KERNEL);
> ... therefore any further NUL-padding is rendered useless.
>
> 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.
>
> I opted not to switch NVMF_NQN_SIZE to sizeof(data->xyz) because the
> size is defined as:
> | /* NQN names in commands fields specified one size */
> | #define NVMF_NQN_FIELD_LEN 256
>
> ... while NVMF_NQN_SIZE is defined as:
> | /* However the max length of a qualified name is another size */
> | #define NVMF_NQN_SIZE 223
>
> Since 223 seems pretty magic, I'm not going to touch it.
struct nvmf_connect_data {
...
char subsysnqn[NVMF_NQN_FIELD_LEN];
char hostnqn[NVMF_NQN_FIELD_LEN];
Honestly, the use of NVMF_NQN_SIZE as the length arg looks like a bug.
struct nvmf_ctrl_options {
...
char *subsysnqn;
...
struct nvmf_host *host;
struct nvmf_host {
...
char nqn[NVMF_NQN_SIZE];
ctrl->opts->host->nqn is sized as NVMF_NQN_SIZE, so this is like saying:
strscpy(dest, src, sizeof(src));
Which can go wrong when dest is smaller than src, but that's not the
case here. It seems ctrl->opts->host->nqn is expected to be a C string:
drivers/nvme/host/fabrics.h: strcmp(opts->host->nqn,
ctrl->opts->host->nqn) ||
And subsysnqn seems to be the same size:
drivers/nvme/target/core.c: subsys->subsysnqn = kstrndup(subsysnqn,
NVMF_NQN_SIZE,
drivers/nvme/target/core.c- GFP_KERNEL);
So these really look like bugs to me. Perhaps a follow-up patch to fix
this, if nvme maintainers agree?
Either way, strscpy is an improvement:
Reviewed-by: Kees Cook <keesc...@chromium.org>
-Kees
>
> 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 <justinst...@google.com>
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
> drivers/nvme/host/fabrics.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 8175d49f2909..57aad3ce311a 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -386,8 +386,8 @@ static struct nvmf_connect_data
> *nvmf_connect_data_prep(struct nvme_ctrl *ctrl,
>
> uuid_copy(&data->hostid, &ctrl->opts->host->id);
> data->cntlid = cpu_to_le16(cntlid);
> - strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
> - strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
> + strscpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
> + strscpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
>
> return data;
> }
>
> ---
> base-commit: 58720809f52779dc0f08e53e54b014209d13eebb
> change-id: 20231018-strncpy-drivers-nvme-host-fabrics-c-416258a22598
>
> Best regards,
> --
> Justin Stitt <justinst...@google.com>
>
>
--
Kees Cook
