Received-SPF: pass (google.com: domain of linux-ext4+bounces-2177-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 24 Apr 2024 16:59:35 -0700
From: Kees Cook <keescook@chromium.org>
To: Theodore Ts'o <tytso@mit.edu>
Cc: Justin Stitt <justinstitt@google.com>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org
Subject: Re: [PATCH] ext4: replace deprecated strncpy with alternatives
Message-ID: <202404241658.E0A17DE2E@keescook>
References: <20240321-strncpy-fs-ext4-file-c-v1-1-36a6a09fef0c@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240321-strncpy-fs-ext4-file-c-v1-1-36a6a09fef0c@google.com>

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