Messages in this thread Patch in this message | | | Date | Sat, 7 Jul 2018 16:01:40 +0200 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCHv2] Fix range checks in kernfs_get_target_path |
| |
On Sat, Jul 07, 2018 at 09:41:03AM +0000, Bernd Edlinger wrote: > The strncpy causes a warning [-Wstringop-truncation] here, > which indicates that it never appends a NUL byte to the path. > The NUL byte is only there because the buffer is allocated > with kzalloc(PAGE_SIZE, GFP_KERNEL), but since the range-check > is also off-by-one, and PAGE_SIZE==PATH_MAX the returned string > will not be zero-terminated if it is exactly PATH_MAX characters. > Furthermore also the initial loop may theoretically exceed PATH_MAX > and cause a fault. > > Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.de> > --- > fs/kernfs/symlink.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/fs/kernfs/symlink.c b/fs/kernfs/symlink.c > index 08ccabd..c8b7d44a 100644 > --- a/fs/kernfs/symlink.c > +++ b/fs/kernfs/symlink.c > @@ -63,7 +63,10 @@ static int kernfs_get_target_path(struct kernfs_node > if (base == kn) > break; > > - strcpy(s, "../"); > + if ((s - path) + 3 >= PATH_MAX) > + return -ENAMETOOLONG; > + > + memcpy(s, "../", 3); > s += 3; > base = base->parent; > } > @@ -79,16 +82,17 @@ static int kernfs_get_target_path(struct kernfs_node > if (len < 2) > return -EINVAL; > len--; > - if ((s - path) + len > PATH_MAX) > + if ((s - path) + len >= PATH_MAX) > return -ENAMETOOLONG; > > /* reverse fillup of target string from target to base */ > kn = target; > + s[len] = '\0'; > while (kn->parent && kn != base) { > int slen = strlen(kn->name); > > len -= slen; > - strncpy(s + len, kn->name, slen); > + memcpy(s + len, kn->name, slen); > if (len) > s[--len] = '/'; >
This last memcpy replacement has already been applied to my tree, from a patch from soeone else, so are you sure all of the other changes are also really needed? Why the extra \0 termination of a string that is already terminated?
And why is the first memcpy replacement needed? gcc doesn't say anything about that, does it?
thanks,
greg k-h
| |