Messages in this thread |  | | Date | Mon, 23 Jan 2023 04:45:04 +0000 | Subject | Re: [PATCH linux-next] Squashfs: use strscpy() to instead of strncpy() | From | Phillip Lougher <> |
| |
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,
|  |