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@xxxxxxxxxxxxxxx > Signed-off-by: Justin Stitt <justinstitt@xxxxxxxxxx> > --- > 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@xxxxxxxxxxxx> -- Kees Cook