[PATCHv2] Fix range checks in kernfs_get_target_path

[PATCHv2] Fix range checks in kernfs_get_target_path

[Bernd Edlinger]

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] = '/';

-- 
1.9.1

Re: [PATCHv2] Fix range checks in kernfs_get_target_path

[Greg Kroah-Hartman]

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

Re: [PATCHv2] Fix range checks in kernfs_get_target_path

[Bernd Edlinger]

On 07/07/18 16:01, Greg Kroah-Hartman wrote:
> 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?
> 

I did only a code review, but the range checks look really dangerously
wrong.

The string is only zero-terminated because it is allocated in
kernfs_iop_get_link with body = kzalloc(PAGE_SIZE, GFP_KERNEL);

I would recommend to explicitly place a termination in the
buffer, and not rely on the way how the buffer is allocated.

> And why is the first memcpy replacement needed?  gcc doesn't say
> anything about that, does it?
> 

No, that is more or less for efficiency reasons, since it is writing
the NUL which is always overwritten with something different in the
next step.  If the loop is executed zero times, there result is not
explicitly zero-terminated either, so using strcpy is somehow misleading.

Well, I would say personal taste, if the loop below constructs
the string with memcpy the loop above can do the same.

If you prefer the strcpy in the first loop, I have no strong
preference here.


Thanks
Bernd.

> thanks,
> 
> greg k-h
> 

Re: [PATCHv2] Fix range checks in kernfs_get_target_path

[Greg Kroah-Hartman]

On Sat, Jul 07, 2018 at 02:34:05PM +0000, Bernd Edlinger wrote:
> On 07/07/18 16:01, Greg Kroah-Hartman wrote:
> > 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?
> > 
> 
> I did only a code review, but the range checks look really dangerously
> wrong.
> 
> The string is only zero-terminated because it is allocated in
> kernfs_iop_get_link with body = kzalloc(PAGE_SIZE, GFP_KERNEL);

Which is why it is allocated that way :)

> I would recommend to explicitly place a termination in the
> buffer, and not rely on the way how the buffer is allocated.

Why not?  We explicitly wanted the buffer created this way, so that we
do not have to work out where to put the termination.  We do that all
the time in the kernel.

> > And why is the first memcpy replacement needed?  gcc doesn't say
> > anything about that, does it?
> > 
> 
> No, that is more or less for efficiency reasons, since it is writing
> the NUL which is always overwritten with something different in the
> next step.  If the loop is executed zero times, there result is not
> explicitly zero-terminated either, so using strcpy is somehow misleading.
> 
> Well, I would say personal taste, if the loop below constructs
> the string with memcpy the loop above can do the same.
> 
> If you prefer the strcpy in the first loop, I have no strong
> preference here.

I'd prefer to change the least amount of code as possible :)

thanks,

greg k-h