Re: [PATCH] NFS: fix nfs_parse_ip_address() corner case

["J. Bruce Fields" <bfields@fieldses.org>]

On Tue, Aug 26, 2008 at 04:24:12PM -0400, Chuck Lever wrote:
> On Aug 26, 2008, at Aug 26, 2008, 2:39 PM, J. Bruce Fields wrote:
>> On Fri, Aug 22, 2008 at 02:24:22PM -0400, Chuck Lever wrote:
>>> Bruce observed that nfs_parse_ip_address() will successfully parse  
>>> an IPv6
>>> address that looks like this:
>>>
>>>  "::1%"
>>>
>>> A scope delimiter is present, but there is no scope ID following it.
>>> This is harmless, as it would simply set the scope ID to zero.   
>>> However,
>>> in some cases we would like to flag this as an improperly formed
>>> address.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/nfs/super.c |   24 +++++++++++++++---------
>>> 1 files changed, 15 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
>>> index 5b2aa04..f73e068 100644
>>> --- a/fs/nfs/super.c
>>> +++ b/fs/nfs/super.c
>>> @@ -727,19 +727,21 @@ static void nfs_parse_ipv4_address(char  
>>> *string, size_t str_len,
>>> #define IPV6_SCOPE_DELIMITER	'%'
>>>
>>> #if defined(CONFIG_IPV6) || defined(CONFIG_IPV6_MODULE)
>>> -static void nfs_parse_ipv6_scope_id(const char *string, const  
>>> size_t str_len,
>>> -				    const char *delim,
>>> -				    struct sockaddr_in6 *sin6)
>>> +static int nfs_parse_ipv6_scope_id(const char *string, const size_t 
>>> str_len,
>>> +				   const char *delim,
>>> +				   struct sockaddr_in6 *sin6)
>>> {
>>> 	char *p;
>>> 	size_t len;
>>>
>>> 	if (!(ipv6_addr_type(&sin6->sin6_addr) & IPV6_ADDR_LINKLOCAL))
>>> -		return ;
>>> +		return 0;
>>> 	if (*delim != IPV6_SCOPE_DELIMITER)
>>> -		return;
>>> -
>>> +		return 0;
>>
>> What happens in the case where there's no scope delimiter?  In that  
>> case
>> can't *delim correctly point to something else here?
>
> When we get to nfs_parse_ipv6_scope_id(), *delim points to the first  
> character following the 128-bit IPv6 address string.  We should fail if 
> *delim doesn't point to either '%' or '\0'.  So we need another check 
> here -- succeed immediately if *delim points to '\0'.

The string isn't necessarily null-delimited.

> Then, I think we should check if the address is link-local _after_ we  
> know we have a valid scope delimiter.
>
>> Arguably kstrndup() and dev_get_by_name() failures should also result 
>> in
>> parser failures.  It seems safer to me to reject bad addresses than to
>> try to use them anyway (possibly resulting in mounting a different
>> server from what was intended).
>
> Well, if kstrndup() fails, that doesn't necessarily mean we have a bad  
> address; simply that there wasn't memory to parse it.  But it's  
> reasonable to return 0 in that case.
>
> If dev_get_by_name() fails, then the next step is to check if we were  
> passed a numeric value instead of a device name.  If the strtoul() call 
> fails to find a real numeric there, then yes, address parsing should 
> fail.

What does %numeric-value mean?

--b.

>
> If you agree I will repost with corrections.