Message-ID: <47e9f2c3-aeea-4d3a-f27f-8ace15cb9a21@squashfs.org.uk>
Date:   Mon, 23 Jan 2023 04:45:04 +0000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH linux-next] Squashfs: use strscpy() to instead of
 strncpy()
To:     yang.yang29@zte.com.cn
Cc:     linux-kernel@vger.kernel.org, xu.panda@zte.com.cn
References: <202301091937558399800@zte.com.cn>
From:   Phillip Lougher <phillip@squashfs.org.uk>
In-Reply-To: <202301091937558399800@zte.com.cn>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 09/01/2023 11:37, yang.yang29@zte.com.cn wrote:
> From: Xu Panda <xu.panda@zte.com.cn>
> 
> The implementation of strscpy() is more robust and safer.
> That's now the recommended way to copy NUL-terminated strings.
> 

NACK.

I have spent quite some time reviewing this patch, and the reasons
are below.

The source name (from the dentry) is passed as pointer and length,
and the code should not assume it is NUL terminated.

Strscpy() will always access len + 1 bytes from the source string,
to check for the NUL terminator and return either the number of
characters copied or -E2BIG if no NUL terminator was found at
name[len].

This means Strscpy() will perform an out of bounds access on the
source string if it is not NUL terminated.  This is incorrect in
itself, and there are no guarantees it won't fail, for example
it might cross a page boundary and the next page may not be
mapped.

The current code is actually a lot safer because it does not assume
the string is NUL terminated.

See the LWN article "strscpy() and the hazards of improved interfaces"
for a discussion of the dangers of conversion patches.

https://lwn.net/Articles/659214/

In particular a quote from Linus Torvalds.

"So why did I waffle about this for so long?

Every time we introduce a new-and-improved interface, people start doing 
these interminable series of trivial conversion patches.

And every time that happens, somebody does some silly mistake, and the 
conversion patch to the improved interface actually makes things worse. 
Because the patch is mindnumbing and trivial, nobody has the attention 
span to look at it carefully, and it's usually done over large swatches 
of source code which means that not every conversion gets tested."

and to quote from the article itself

"It is there to be used with new code, but existing code should not be 
converted without some compelling reason to do so — or without a high 
level of attention to the possible implications of the change."

Which is why I'm wary of these apparently trivial changes, and I have 
spent quite some time reviewing this patch.

Phillip

> Signed-off-by: Xu Panda <xu.panda@zte.com.cn>
> Signed-off-by: Yang Yang <yang.yang29@zte.com.cn>
> ---
>   fs/squashfs/namei.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/squashfs/namei.c b/fs/squashfs/namei.c
> index 11e4539b9eae..6c4704ba8f42 100644
> --- a/fs/squashfs/namei.c
> +++ b/fs/squashfs/namei.c
> @@ -80,8 +80,7 @@ static int get_dir_index_using_name(struct super_block *sb,
>   	}
> 
>   	str = &index->name[SQUASHFS_NAME_LEN + 1];
> -	strncpy(str, name, len);
> -	str[len] = '\0';
> +	strscpy(str, name, len + 1);
> 
>   	for (i = 0; i < i_count; i++) {
>   		err = squashfs_read_metadata(sb, index, &index_start,