* [PATCH] ext4: replace deprecated strncpy with alternatives
@ 2024-03-21 1:03 Justin Stitt
2024-03-29 4:11 ` Kees Cook
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Justin Stitt @ 2024-03-21 1:03 UTC (permalink / raw)
To: Theodore Ts'o, Andreas Dilger
Cc: linux-ext4, linux-kernel, linux-hardening, Justin Stitt
strncpy() is deprecated for use on NUL-terminated destination strings
[1] and as such we should prefer more robust and less ambiguous string
interfaces.
in file.c:
s_last_mounted is marked as __nonstring meaning it does not need to be
NUL-terminated. Let's instead use strtomem_pad() to copy bytes from the
string source to the byte array destination -- while also ensuring to
pad with zeroes.
in ioctl.c:
We can drop the memset and size argument in favor of using the new
2-argument version of strscpy_pad() -- which was introduced with Commit
e6584c3964f2f ("string: Allow 2-argument strscpy()"). This guarantees
NUL-termination and NUL-padding on the destination buffer -- which seems
to be a requirement judging from this comment:
| static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label)
| {
| char label[EXT4_LABEL_MAX + 1];
|
| /*
| * EXT4_LABEL_MAX must always be smaller than FSLABEL_MAX because
| * FSLABEL_MAX must include terminating null byte, while s_volume_name
| * does not have to.
| */
in super.c:
s_first_error_func is marked as __nonstring meaning we can take the same
approach as in file.c; just use strtomem_pad()
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\("
---
fs/ext4/file.c | 3 +--
fs/ext4/ioctl.c | 3 +--
fs/ext4/super.c | 7 +++----
3 files changed, 5 insertions(+), 8 deletions(-)
diff --git a/fs/ext4/file.c b/fs/ext4/file.c
index 54d6ff22585c..c675c0eb5f7e 100644
--- a/fs/ext4/file.c
+++ b/fs/ext4/file.c
@@ -844,8 +844,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
if (err)
goto out_journal;
lock_buffer(sbi->s_sbh);
- strncpy(sbi->s_es->s_last_mounted, cp,
- sizeof(sbi->s_es->s_last_mounted));
+ strtomem_pad(sbi->s_es->s_last_mounted, cp, 0);
ext4_superblock_csum_set(sb);
unlock_buffer(sbi->s_sbh);
ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 7160a71044c8..dab7acd49709 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -1150,9 +1150,8 @@ static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label
*/
BUILD_BUG_ON(EXT4_LABEL_MAX >= FSLABEL_MAX);
- memset(label, 0, sizeof(label));
lock_buffer(sbi->s_sbh);
- strncpy(label, sbi->s_es->s_volume_name, EXT4_LABEL_MAX);
+ strscpy_pad(label, sbi->s_es->s_volume_name);
unlock_buffer(sbi->s_sbh);
if (copy_to_user(user_label, label, sizeof(label)))
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cfb8449c731f..de16d045ca05 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -6130,8 +6130,8 @@ static void ext4_update_super(struct super_block *sb)
__ext4_update_tstamp(&es->s_first_error_time,
&es->s_first_error_time_hi,
sbi->s_first_error_time);
- strncpy(es->s_first_error_func, sbi->s_first_error_func,
- sizeof(es->s_first_error_func));
+ strtomem_pad(es->s_first_error_func,
+ sbi->s_first_error_func, 0);
es->s_first_error_line =
cpu_to_le32(sbi->s_first_error_line);
es->s_first_error_ino =
@@ -6144,8 +6144,7 @@ static void ext4_update_super(struct super_block *sb)
__ext4_update_tstamp(&es->s_last_error_time,
&es->s_last_error_time_hi,
sbi->s_last_error_time);
- strncpy(es->s_last_error_func, sbi->s_last_error_func,
- sizeof(es->s_last_error_func));
+ strtomem_pad(es->s_last_error_func, sbi->s_last_error_func, 0);
es->s_last_error_line = cpu_to_le32(sbi->s_last_error_line);
es->s_last_error_ino = cpu_to_le32(sbi->s_last_error_ino);
es->s_last_error_block = cpu_to_le64(sbi->s_last_error_block);
---
base-commit: a4145ce1e7bc247fd6f2846e8699473448717b37
change-id: 20240321-strncpy-fs-ext4-file-c-c983d942dd39
Best regards,
--
Justin Stitt <justinstitt@google.com>
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: replace deprecated strncpy with alternatives
2024-03-21 1:03 [PATCH] ext4: replace deprecated strncpy with alternatives Justin Stitt
@ 2024-03-29 4:11 ` Kees Cook
2024-04-24 23:59 ` Kees Cook
2024-05-03 4:01 ` Theodore Ts'o
2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2024-03-29 4:11 UTC (permalink / raw)
To: Justin Stitt
Cc: Theodore Ts'o, Andreas Dilger, linux-ext4, linux-kernel,
linux-hardening
On Thu, Mar 21, 2024 at 01:03:10AM +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.
>
> in file.c:
> s_last_mounted is marked as __nonstring meaning it does not need to be
> NUL-terminated. Let's instead use strtomem_pad() to copy bytes from the
> string source to the byte array destination -- while also ensuring to
> pad with zeroes.
>
> in ioctl.c:
> We can drop the memset and size argument in favor of using the new
> 2-argument version of strscpy_pad() -- which was introduced with Commit
> e6584c3964f2f ("string: Allow 2-argument strscpy()"). This guarantees
> NUL-termination and NUL-padding on the destination buffer -- which seems
> to be a requirement judging from this comment:
>
> | static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label)
> | {
> | char label[EXT4_LABEL_MAX + 1];
> |
> | /*
> | * EXT4_LABEL_MAX must always be smaller than FSLABEL_MAX because
> | * FSLABEL_MAX must include terminating null byte, while s_volume_name
> | * does not have to.
> | */
>
> in super.c:
> s_first_error_func is marked as __nonstring meaning we can take the same
> approach as in file.c; just use strtomem_pad()
>
> 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\("
> ---
> fs/ext4/file.c | 3 +--
> fs/ext4/ioctl.c | 3 +--
> fs/ext4/super.c | 7 +++----
> 3 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 54d6ff22585c..c675c0eb5f7e 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -844,8 +844,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
> if (err)
> goto out_journal;
> lock_buffer(sbi->s_sbh);
> - strncpy(sbi->s_es->s_last_mounted, cp,
> - sizeof(sbi->s_es->s_last_mounted));
> + strtomem_pad(sbi->s_es->s_last_mounted, cp, 0);
> ext4_superblock_csum_set(sb);
> unlock_buffer(sbi->s_sbh);
> ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 7160a71044c8..dab7acd49709 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1150,9 +1150,8 @@ static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label
> */
> BUILD_BUG_ON(EXT4_LABEL_MAX >= FSLABEL_MAX);
>
> - memset(label, 0, sizeof(label));
> lock_buffer(sbi->s_sbh);
> - strncpy(label, sbi->s_es->s_volume_name, EXT4_LABEL_MAX);
> + strscpy_pad(label, sbi->s_es->s_volume_name);
> unlock_buffer(sbi->s_sbh);
The only reason I can imagine the memset() being split here is to keep
it out of the spinlock. For a non-fast-path ioctl, I can't imagine this
being a meaningful reduction in lock contention, though.
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: replace deprecated strncpy with alternatives
2024-03-21 1:03 [PATCH] ext4: replace deprecated strncpy with alternatives Justin Stitt
2024-03-29 4:11 ` Kees Cook
@ 2024-04-24 23:59 ` Kees Cook
2024-05-03 4:01 ` Theodore Ts'o
2 siblings, 0 replies; 4+ messages in thread
From: Kees Cook @ 2024-04-24 23:59 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Justin Stitt, Andreas Dilger, linux-ext4, linux-kernel, linux-hardening
On Thu, Mar 21, 2024 at 01:03:10AM +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.
>
> in file.c:
> s_last_mounted is marked as __nonstring meaning it does not need to be
> NUL-terminated. Let's instead use strtomem_pad() to copy bytes from the
> string source to the byte array destination -- while also ensuring to
> pad with zeroes.
>
> in ioctl.c:
> We can drop the memset and size argument in favor of using the new
> 2-argument version of strscpy_pad() -- which was introduced with Commit
> e6584c3964f2f ("string: Allow 2-argument strscpy()"). This guarantees
> NUL-termination and NUL-padding on the destination buffer -- which seems
> to be a requirement judging from this comment:
>
> | static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label)
> | {
> | char label[EXT4_LABEL_MAX + 1];
> |
> | /*
> | * EXT4_LABEL_MAX must always be smaller than FSLABEL_MAX because
> | * FSLABEL_MAX must include terminating null byte, while s_volume_name
> | * does not have to.
> | */
>
> in super.c:
> s_first_error_func is marked as __nonstring meaning we can take the same
> approach as in file.c; just use strtomem_pad()
>
> 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>
Hi Ted, are you able to pick this up for the ext4 tree? If it makes it
easier, I can carry it in my tree instead...
Thanks!
-Kees
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
> fs/ext4/file.c | 3 +--
> fs/ext4/ioctl.c | 3 +--
> fs/ext4/super.c | 7 +++----
> 3 files changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 54d6ff22585c..c675c0eb5f7e 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -844,8 +844,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
> if (err)
> goto out_journal;
> lock_buffer(sbi->s_sbh);
> - strncpy(sbi->s_es->s_last_mounted, cp,
> - sizeof(sbi->s_es->s_last_mounted));
> + strtomem_pad(sbi->s_es->s_last_mounted, cp, 0);
> ext4_superblock_csum_set(sb);
> unlock_buffer(sbi->s_sbh);
> ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 7160a71044c8..dab7acd49709 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1150,9 +1150,8 @@ static int ext4_ioctl_getlabel(struct ext4_sb_info *sbi, char __user *user_label
> */
> BUILD_BUG_ON(EXT4_LABEL_MAX >= FSLABEL_MAX);
>
> - memset(label, 0, sizeof(label));
> lock_buffer(sbi->s_sbh);
> - strncpy(label, sbi->s_es->s_volume_name, EXT4_LABEL_MAX);
> + strscpy_pad(label, sbi->s_es->s_volume_name);
> unlock_buffer(sbi->s_sbh);
>
> if (copy_to_user(user_label, label, sizeof(label)))
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index cfb8449c731f..de16d045ca05 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6130,8 +6130,8 @@ static void ext4_update_super(struct super_block *sb)
> __ext4_update_tstamp(&es->s_first_error_time,
> &es->s_first_error_time_hi,
> sbi->s_first_error_time);
> - strncpy(es->s_first_error_func, sbi->s_first_error_func,
> - sizeof(es->s_first_error_func));
> + strtomem_pad(es->s_first_error_func,
> + sbi->s_first_error_func, 0);
> es->s_first_error_line =
> cpu_to_le32(sbi->s_first_error_line);
> es->s_first_error_ino =
> @@ -6144,8 +6144,7 @@ static void ext4_update_super(struct super_block *sb)
> __ext4_update_tstamp(&es->s_last_error_time,
> &es->s_last_error_time_hi,
> sbi->s_last_error_time);
> - strncpy(es->s_last_error_func, sbi->s_last_error_func,
> - sizeof(es->s_last_error_func));
> + strtomem_pad(es->s_last_error_func, sbi->s_last_error_func, 0);
> es->s_last_error_line = cpu_to_le32(sbi->s_last_error_line);
> es->s_last_error_ino = cpu_to_le32(sbi->s_last_error_ino);
> es->s_last_error_block = cpu_to_le64(sbi->s_last_error_block);
>
> ---
> base-commit: a4145ce1e7bc247fd6f2846e8699473448717b37
> change-id: 20240321-strncpy-fs-ext4-file-c-c983d942dd39
>
> Best regards,
> --
> Justin Stitt <justinstitt@google.com>
>
--
Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ext4: replace deprecated strncpy with alternatives
2024-03-21 1:03 [PATCH] ext4: replace deprecated strncpy with alternatives Justin Stitt
2024-03-29 4:11 ` Kees Cook
2024-04-24 23:59 ` Kees Cook
@ 2024-05-03 4:01 ` Theodore Ts'o
2 siblings, 0 replies; 4+ messages in thread
From: Theodore Ts'o @ 2024-05-03 4:01 UTC (permalink / raw)
To: Andreas Dilger, Justin Stitt
Cc: Theodore Ts'o, linux-ext4, linux-kernel, linux-hardening
On Thu, 21 Mar 2024 01:03:10 +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.
>
> in file.c:
> s_last_mounted is marked as __nonstring meaning it does not need to be
> NUL-terminated. Let's instead use strtomem_pad() to copy bytes from the
> string source to the byte array destination -- while also ensuring to
> pad with zeroes.
>
> [...]
Applied, thanks!
[1/1] ext4: replace deprecated strncpy with alternatives
commit: 744a56389f7398f286231e062c2e63f0de01bcc6
Best regards,
--
Theodore Ts'o <tytso@mit.edu>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-03 4:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-21 1:03 [PATCH] ext4: replace deprecated strncpy with alternatives Justin Stitt
2024-03-29 4:11 ` Kees Cook
2024-04-24 23:59 ` Kees Cook
2024-05-03 4:01 ` Theodore Ts'o
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.