| Message ID | 20230925-strncpy-drivers-md-dm-cache-metadata-c-v1-1-4b75c7db0cfe@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | ac4149ba7efd6bc327c1e15e812091984f3a16b2 |
| Headers | show |
| Series | dm cache metadata: replace deprecated strncpy with strscpy | expand |
On Mon, Sep 25, 2023 at 06:13:12AM +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. > > It seems `cmd->policy_name` is intended to be NUL-terminated based on a > now changed line of code from Commit (c6b4fcbad044e6ff "dm: add cache > target"): > | if (strcmp(cmd->policy_name, policy_name)) { // ... > > However, now a length-bounded strncmp is used: > | if (strncmp(cmd->policy_name, policy_name, sizeof(cmd->policy_name))) > ... which means NUL-terminated may not strictly be required. However, I > believe the intent of the code is clear and we should maintain > NUL-termination of policy_names. > > Moreover, __begin_transaction_flags() zero-allocates `cmd` before > calling read_superblock_fields(): > | cmd = kzalloc(sizeof(*cmd), GFP_KERNEL); > > Also, `disk_super->policy_name` is zero-initialized > | memset(disk_super->policy_name, 0, sizeof(disk_super->policy_name)); > ... therefore any NUL-padding is redundant. > > 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> Agreed about the %NUL termination and padding assessment. Reviewed-by: Kees Cook <keescook@chromium.org> -Kees
On Mon, 25 Sep 2023 06:13:12 +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. > > It seems `cmd->policy_name` is intended to be NUL-terminated based on a > now changed line of code from Commit (c6b4fcbad044e6ff "dm: add cache > target"): > | if (strcmp(cmd->policy_name, policy_name)) { // ... > > [...] Applied to for-next/hardening, thanks! [1/1] dm cache metadata: replace deprecated strncpy with strscpy https://git.kernel.org/kees/c/5d9bc443188f Take care,
diff --git a/drivers/md/dm-cache-metadata.c b/drivers/md/dm-cache-metadata.c index acffed750e3e..5a18b80d3666 100644 --- a/drivers/md/dm-cache-metadata.c +++ b/drivers/md/dm-cache-metadata.c @@ -597,7 +597,7 @@ static void read_superblock_fields(struct dm_cache_metadata *cmd, cmd->discard_nr_blocks = to_dblock(le64_to_cpu(disk_super->discard_nr_blocks)); cmd->data_block_size = le32_to_cpu(disk_super->data_block_size); cmd->cache_blocks = to_cblock(le32_to_cpu(disk_super->cache_blocks)); - strncpy(cmd->policy_name, disk_super->policy_name, sizeof(cmd->policy_name)); + strscpy(cmd->policy_name, disk_super->policy_name, sizeof(cmd->policy_name)); cmd->policy_version[0] = le32_to_cpu(disk_super->policy_version[0]); cmd->policy_version[1] = le32_to_cpu(disk_super->policy_version[1]); cmd->policy_version[2] = le32_to_cpu(disk_super->policy_version[2]); @@ -707,7 +707,7 @@ static int __commit_transaction(struct dm_cache_metadata *cmd, disk_super->discard_block_size = cpu_to_le64(cmd->discard_block_size); disk_super->discard_nr_blocks = cpu_to_le64(from_dblock(cmd->discard_nr_blocks)); disk_super->cache_blocks = cpu_to_le32(from_cblock(cmd->cache_blocks)); - strncpy(disk_super->policy_name, cmd->policy_name, sizeof(disk_super->policy_name)); + strscpy(disk_super->policy_name, cmd->policy_name, sizeof(disk_super->policy_name)); disk_super->policy_version[0] = cpu_to_le32(cmd->policy_version[0]); disk_super->policy_version[1] = cpu_to_le32(cmd->policy_version[1]); disk_super->policy_version[2] = cpu_to_le32(cmd->policy_version[2]); @@ -1726,7 +1726,7 @@ static int write_hints(struct dm_cache_metadata *cmd, struct dm_cache_policy *po (strlen(policy_name) > sizeof(cmd->policy_name) - 1)) return -EINVAL; - strncpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name)); + strscpy(cmd->policy_name, policy_name, sizeof(cmd->policy_name)); memcpy(cmd->policy_version, policy_version, sizeof(cmd->policy_version)); hint_size = dm_cache_policy_get_hint_size(policy);
`strncpy` is deprecated for use on NUL-terminated destination strings [1] and as such we should prefer more robust and less ambiguous string interfaces. It seems `cmd->policy_name` is intended to be NUL-terminated based on a now changed line of code from Commit (c6b4fcbad044e6ff "dm: add cache target"): | if (strcmp(cmd->policy_name, policy_name)) { // ... However, now a length-bounded strncmp is used: | if (strncmp(cmd->policy_name, policy_name, sizeof(cmd->policy_name))) ... which means NUL-terminated may not strictly be required. However, I believe the intent of the code is clear and we should maintain NUL-termination of policy_names. Moreover, __begin_transaction_flags() zero-allocates `cmd` before calling read_superblock_fields(): | cmd = kzalloc(sizeof(*cmd), GFP_KERNEL); Also, `disk_super->policy_name` is zero-initialized | memset(disk_super->policy_name, 0, sizeof(disk_super->policy_name)); ... therefore any NUL-padding is redundant. 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. --- drivers/md/dm-cache-metadata.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- base-commit: 6465e260f48790807eef06b583b38ca9789b6072 change-id: 20230925-strncpy-drivers-md-dm-cache-metadata-c-f708096d5d22 Best regards, -- Justin Stitt <justinstitt@google.com>