[lkml]   [2023]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH linux-next] Squashfs: use strscpy() to instead of strncpy()
On 09/01/2023 11:37, wrote:
> From: Xu Panda <>
> The implementation of strscpy() is more robust and safer.
> That's now the recommended way to copy NUL-terminated strings.


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

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

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.

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.


> Signed-off-by: Xu Panda <>
> Signed-off-by: Yang Yang <>
> ---
> 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,

 \ /
  Last update: 2023-03-26 23:52    [W:0.055 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site