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 <[email protected]>
---
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;
len = (string + str_len) - delim - 1;
+ if (len == 0)
+ return 0;
+
p = kstrndup(delim + 1, len, GFP_KERNEL);
if (p) {
unsigned long scope_id = 0;
@@ -758,6 +760,8 @@ static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
sin6->sin6_scope_id = scope_id;
dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
}
+
+ return 1;
}
static void nfs_parse_ipv6_address(char *string, size_t str_len,
@@ -773,9 +777,11 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
sin6->sin6_family = AF_INET6;
*addr_len = sizeof(*sin6);
- if (in6_pton(string, str_len, addr, IPV6_SCOPE_DELIMITER, &delim)) {
- nfs_parse_ipv6_scope_id(string, str_len, delim, sin6);
- return;
+ if (in6_pton(string, str_len, addr,
+ IPV6_SCOPE_DELIMITER, &delim)) {
+ if (nfs_parse_ipv6_scope_id(string,
+ str_len, delim, sin6))
+ return;
}
}
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 <[email protected]>
>> ---
>>
>> 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'.
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.
If you agree I will repost with corrections.
>
>
> --b.
>
>> len = (string + str_len) - delim - 1;
>> + if (len == 0)
>> + return 0;
>> +
>> p = kstrndup(delim + 1, len, GFP_KERNEL);
>> if (p) {
>> unsigned long scope_id = 0;
>> @@ -758,6 +760,8 @@ static void nfs_parse_ipv6_scope_id(const char
>> *string, const size_t str_len,
>> sin6->sin6_scope_id = scope_id;
>> dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
>> }
>> +
>> + return 1;
>> }
>>
>> static void nfs_parse_ipv6_address(char *string, size_t str_len,
>> @@ -773,9 +777,11 @@ static void nfs_parse_ipv6_address(char
>> *string, size_t str_len,
>>
>> sin6->sin6_family = AF_INET6;
>> *addr_len = sizeof(*sin6);
>> - if (in6_pton(string, str_len, addr, IPV6_SCOPE_DELIMITER,
>> &delim)) {
>> - nfs_parse_ipv6_scope_id(string, str_len, delim, sin6);
>> - return;
>> + if (in6_pton(string, str_len, addr,
>> + IPV6_SCOPE_DELIMITER, &delim)) {
>> + if (nfs_parse_ipv6_scope_id(string,
>> + str_len, delim, sin6))
>> + return;
>> }
>> }
>>
>>
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
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 <[email protected]>
>>> ---
>>>
>>> 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.
On Aug 26, 2008, at Aug 26, 2008, 4:28 PM, J. Bruce Fields wrote:
> 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 <[email protected]>
>>>> ---
>>>>
>>>> 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.
OK, we just need to take str_len into account.
>> 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?
'%eth0' means find and use the scope ID of the eth0 device. '%2'
means use the scope ID 2. If eth0 has a interface index of 2, then
both of these are equivalent. The numeric index is the scope ID.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Tue, Aug 26, 2008 at 04:36:10PM -0400, Chuck Lever wrote:
> On Aug 26, 2008, at Aug 26, 2008, 4:28 PM, J. Bruce Fields wrote:
>> 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 <[email protected]>
>>>>> ---
>>>>>
>>>>> 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.
>
> OK, we just need to take str_len into account.
OK. A minor nit, but I'd also find this a little easier to read if it
attempted to stick to the pattern
if (something_bad)
fail;
if (something_else_bad)
fail;
...
succeed;
rather than
if (successful) {
if (still_successful) {
...
succeed;
}
}
fail;
--b.
>
>>> 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?
>
> '%eth0' means find and use the scope ID of the eth0 device. '%2' means
> use the scope ID 2. If eth0 has a interface index of 2, then both of
> these are equivalent. The numeric index is the scope ID.
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
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 <[email protected]>
> ---
>
> 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?
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).
--b.
> len = (string + str_len) - delim - 1;
> + if (len == 0)
> + return 0;
> +
> p = kstrndup(delim + 1, len, GFP_KERNEL);
> if (p) {
> unsigned long scope_id = 0;
> @@ -758,6 +760,8 @@ static void nfs_parse_ipv6_scope_id(const char *string, const size_t str_len,
> sin6->sin6_scope_id = scope_id;
> dfprintk(MOUNT, "NFS: IPv6 scope ID = %lu\n", scope_id);
> }
> +
> + return 1;
> }
>
> static void nfs_parse_ipv6_address(char *string, size_t str_len,
> @@ -773,9 +777,11 @@ static void nfs_parse_ipv6_address(char *string, size_t str_len,
>
> sin6->sin6_family = AF_INET6;
> *addr_len = sizeof(*sin6);
> - if (in6_pton(string, str_len, addr, IPV6_SCOPE_DELIMITER, &delim)) {
> - nfs_parse_ipv6_scope_id(string, str_len, delim, sin6);
> - return;
> + if (in6_pton(string, str_len, addr,
> + IPV6_SCOPE_DELIMITER, &delim)) {
> + if (nfs_parse_ipv6_scope_id(string,
> + str_len, delim, sin6))
> + return;
> }
> }
>
>