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@xxxxxxxxxx>
>> ---
>>    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
>