2018-06-05 15:51:28

by Olga Kornievskaia

[permalink] [raw]
Subject: [PATCH v2 1/1] Add check of clientaddr argument

If the user supplies a clientaddr value, it should be either
a special value of either IPv4/IPv6 any address or one of the
machine's network addresses. Otherwise, the use of an arbitrary
value of the clientaddr value is not allowed.

Signed-off-by: Olga Kornievskaia <[email protected]>
---
utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
utils/mount/network.h | 2 ++
utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
3 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index e490399..2a54f82 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -50,6 +50,8 @@
#include <rpc/rpc.h>
#include <rpc/pmap_prot.h>
#include <rpc/pmap_clnt.h>
+#include <net/if.h>
+#include <ifaddrs.h>

#include "sockaddr.h"
#include "xcommon.h"
@@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options *options,

return EX_SUCCESS;
}
+
+int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
+{
+ switch (nfs_saddr->sa_family) {
+ case AF_INET: {
+ if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
+ INADDR_ANY)
+ return 1;
+ }
+ case AF_INET6:
+ if (!memcmp(&((struct sockaddr_in6 *)nfs_saddr)->sin6_addr,
+ &in6addr_any, sizeof(in6addr_any)))
+ return 1;
+ }
+ return 0;
+}
+
+int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
+{
+ struct ifaddrs *myaddrs, *ifa;
+ int found = 0;
+
+ /* acquire exiting network interfaces */
+ if (getifaddrs(&myaddrs) != 0)
+ return 0;
+
+ /* interate over the available interfaces and check if we
+ * we find a match to the supplied clientaddr value
+ */
+ for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
+ if (ifa->ifa_addr == NULL)
+ continue;
+ if (!(ifa->ifa_flags & IFF_UP))
+ continue;
+ if (!memcmp(ifa->ifa_addr, nfs_saddr,
+ sizeof(struct sockaddr))) {
+ found = 1;
+ break;
+ }
+ }
+ freeifaddrs(myaddrs);
+ return found;
+}
diff --git a/utils/mount/network.h b/utils/mount/network.h
index ecaac33..0fc98ac 100644
--- a/utils/mount/network.h
+++ b/utils/mount/network.h
@@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const socklen_t,
int clnt_ping(struct sockaddr_in *, const unsigned long,
const unsigned long, const unsigned int,
struct sockaddr_in *);
+int nfs_is_inaddr_any(struct sockaddr *);
+int nfs_addr_matches_localips(struct sockaddr *);

struct mount_options;

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index d1b0708..e22216f 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct sockaddr *sap,

/*
* Called to discover our address and append an appropriate 'clientaddr='
- * option to the options string.
+ * option to the options string. If the supplied 'clientaddr=' value does
+ * not match either IPV4/IPv6 any or a local address, then fail the mount.
*
* Returns 1 if 'clientaddr=' option created successfully or if
* 'clientaddr=' option is already present; otherwise zero.
@@ -242,8 +243,29 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
struct sockaddr *my_addr = &address.sa;
socklen_t my_len = sizeof(address);

- if (po_contains(options, "clientaddr") == PO_FOUND)
- return 1;
+ if (po_contains(options, "clientaddr") == PO_FOUND) {
+ char *addr = po_get(options, "clientaddr");
+ union nfs_sockaddr nfs_address;
+ struct sockaddr *nfs_saddr = &nfs_address.sa;
+ socklen_t nfs_salen = sizeof(nfs_address);
+
+ /* translate the input for clientaddr to nfs_sockaddr */
+ if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
+ return 0;
+
+ /* check for IPV4_ANY and IPV6_ANY */
+ if (nfs_is_inaddr_any(nfs_saddr))
+ return 1;
+
+ /* check if ip matches local network addresses */
+ if (!nfs_addr_matches_localips(nfs_saddr)) {
+ nfs_error(_("%s: supplied clientaddr=%s does not "
+ "match any existing network addresses"),
+ progname, addr);
+ return 0;
+ } else
+ return 1;
+ }

nfs_callback_address(sap, salen, my_addr, &my_len);

--
1.8.3.1



2018-06-05 15:53:59

by Ben Greear

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] Add check of clientaddr argument

On 06/05/2018 08:51 AM, Olga Kornievskaia wrote:
> If the user supplies a clientaddr value, it should be either
> a special value of either IPv4/IPv6 any address or one of the
> machine's network addresses. Otherwise, the use of an arbitrary
> value of the clientaddr value is not allowed.

Is this finally going to let one bind to a local IP address?

I have patches to do this for years now, would love to get that
feature upstream!

Thanks,
Ben

>
> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> utils/mount/network.h | 2 ++
> utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
> 3 files changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index e490399..2a54f82 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -50,6 +50,8 @@
> #include <rpc/rpc.h>
> #include <rpc/pmap_prot.h>
> #include <rpc/pmap_clnt.h>
> +#include <net/if.h>
> +#include <ifaddrs.h>
>
> #include "sockaddr.h"
> #include "xcommon.h"
> @@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options *options,
>
> return EX_SUCCESS;
> }
> +
> +int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
> +{
> + switch (nfs_saddr->sa_family) {
> + case AF_INET: {
> + if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
> + INADDR_ANY)
> + return 1;
> + }
> + case AF_INET6:
> + if (!memcmp(&((struct sockaddr_in6 *)nfs_saddr)->sin6_addr,
> + &in6addr_any, sizeof(in6addr_any)))
> + return 1;
> + }
> + return 0;
> +}
> +
> +int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
> +{
> + struct ifaddrs *myaddrs, *ifa;
> + int found = 0;
> +
> + /* acquire exiting network interfaces */
> + if (getifaddrs(&myaddrs) != 0)
> + return 0;
> +
> + /* interate over the available interfaces and check if we
> + * we find a match to the supplied clientaddr value
> + */
> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
> + if (ifa->ifa_addr == NULL)
> + continue;
> + if (!(ifa->ifa_flags & IFF_UP))
> + continue;
> + if (!memcmp(ifa->ifa_addr, nfs_saddr,
> + sizeof(struct sockaddr))) {
> + found = 1;
> + break;
> + }
> + }
> + freeifaddrs(myaddrs);
> + return found;
> +}
> diff --git a/utils/mount/network.h b/utils/mount/network.h
> index ecaac33..0fc98ac 100644
> --- a/utils/mount/network.h
> +++ b/utils/mount/network.h
> @@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const socklen_t,
> int clnt_ping(struct sockaddr_in *, const unsigned long,
> const unsigned long, const unsigned int,
> struct sockaddr_in *);
> +int nfs_is_inaddr_any(struct sockaddr *);
> +int nfs_addr_matches_localips(struct sockaddr *);
>
> struct mount_options;
>
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index d1b0708..e22216f 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
>
> /*
> * Called to discover our address and append an appropriate 'clientaddr='
> - * option to the options string.
> + * option to the options string. If the supplied 'clientaddr=' value does
> + * not match either IPV4/IPv6 any or a local address, then fail the mount.
> *
> * Returns 1 if 'clientaddr=' option created successfully or if
> * 'clientaddr=' option is already present; otherwise zero.
> @@ -242,8 +243,29 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
> struct sockaddr *my_addr = &address.sa;
> socklen_t my_len = sizeof(address);
>
> - if (po_contains(options, "clientaddr") == PO_FOUND)
> - return 1;
> + if (po_contains(options, "clientaddr") == PO_FOUND) {
> + char *addr = po_get(options, "clientaddr");
> + union nfs_sockaddr nfs_address;
> + struct sockaddr *nfs_saddr = &nfs_address.sa;
> + socklen_t nfs_salen = sizeof(nfs_address);
> +
> + /* translate the input for clientaddr to nfs_sockaddr */
> + if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
> + return 0;
> +
> + /* check for IPV4_ANY and IPV6_ANY */
> + if (nfs_is_inaddr_any(nfs_saddr))
> + return 1;
> +
> + /* check if ip matches local network addresses */
> + if (!nfs_addr_matches_localips(nfs_saddr)) {
> + nfs_error(_("%s: supplied clientaddr=%s does not "
> + "match any existing network addresses"),
> + progname, addr);
> + return 0;
> + } else
> + return 1;
> + }
>
> nfs_callback_address(sap, salen, my_addr, &my_len);
>
>


--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com


2018-06-05 16:05:18

by Olga Kornievskaia

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] Add check of clientaddr argument

On Tue, Jun 5, 2018 at 11:53 AM, Ben Greear <[email protected]> wrote:
> On 06/05/2018 08:51 AM, Olga Kornievskaia wrote:
>>
>> If the user supplies a clientaddr value, it should be either
>> a special value of either IPv4/IPv6 any address or one of the
>> machine's network addresses. Otherwise, the use of an arbitrary
>> value of the clientaddr value is not allowed.
>
>
> Is this finally going to let one bind to a local IP address?

No this patch only checks for the validity of the arguments for the
callback channel.

> I have patches to do this for years now, would love to get that
> feature upstream!

Have you posted those patches to the mailing list?

>
> Thanks,
> Ben
>
>
>>
>> Signed-off-by: Olga Kornievskaia <[email protected]>
>> ---
>> utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> utils/mount/network.h | 2 ++
>> utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
>> 3 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>> index e490399..2a54f82 100644
>> --- a/utils/mount/network.c
>> +++ b/utils/mount/network.c
>> @@ -50,6 +50,8 @@
>> #include <rpc/rpc.h>
>> #include <rpc/pmap_prot.h>
>> #include <rpc/pmap_clnt.h>
>> +#include <net/if.h>
>> +#include <ifaddrs.h>
>>
>> #include "sockaddr.h"
>> #include "xcommon.h"
>> @@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options
>> *options,
>>
>> return EX_SUCCESS;
>> }
>> +
>> +int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
>> +{
>> + switch (nfs_saddr->sa_family) {
>> + case AF_INET: {
>> + if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
>> + INADDR_ANY)
>> + return 1;
>> + }
>> + case AF_INET6:
>> + if (!memcmp(&((struct sockaddr_in6
>> *)nfs_saddr)->sin6_addr,
>> + &in6addr_any, sizeof(in6addr_any)))
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +
>> +int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
>> +{
>> + struct ifaddrs *myaddrs, *ifa;
>> + int found = 0;
>> +
>> + /* acquire exiting network interfaces */
>> + if (getifaddrs(&myaddrs) != 0)
>> + return 0;
>> +
>> + /* interate over the available interfaces and check if we
>> + * we find a match to the supplied clientaddr value
>> + */
>> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
>> + if (ifa->ifa_addr == NULL)
>> + continue;
>> + if (!(ifa->ifa_flags & IFF_UP))
>> + continue;
>> + if (!memcmp(ifa->ifa_addr, nfs_saddr,
>> + sizeof(struct sockaddr))) {
>> + found = 1;
>> + break;
>> + }
>> + }
>> + freeifaddrs(myaddrs);
>> + return found;
>> +}
>> diff --git a/utils/mount/network.h b/utils/mount/network.h
>> index ecaac33..0fc98ac 100644
>> --- a/utils/mount/network.h
>> +++ b/utils/mount/network.h
>> @@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const
>> socklen_t,
>> int clnt_ping(struct sockaddr_in *, const unsigned long,
>> const unsigned long, const unsigned int,
>> struct sockaddr_in *);
>> +int nfs_is_inaddr_any(struct sockaddr *);
>> +int nfs_addr_matches_localips(struct sockaddr *);
>>
>> struct mount_options;
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index d1b0708..e22216f 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct
>> sockaddr *sap,
>>
>> /*
>> * Called to discover our address and append an appropriate 'clientaddr='
>> - * option to the options string.
>> + * option to the options string. If the supplied 'clientaddr=' value does
>> + * not match either IPV4/IPv6 any or a local address, then fail the
>> mount.
>> *
>> * Returns 1 if 'clientaddr=' option created successfully or if
>> * 'clientaddr=' option is already present; otherwise zero.
>> @@ -242,8 +243,29 @@ static int nfs_append_clientaddr_option(const struct
>> sockaddr *sap,
>> struct sockaddr *my_addr = &address.sa;
>> socklen_t my_len = sizeof(address);
>>
>> - if (po_contains(options, "clientaddr") == PO_FOUND)
>> - return 1;
>> + if (po_contains(options, "clientaddr") == PO_FOUND) {
>> + char *addr = po_get(options, "clientaddr");
>> + union nfs_sockaddr nfs_address;
>> + struct sockaddr *nfs_saddr = &nfs_address.sa;
>> + socklen_t nfs_salen = sizeof(nfs_address);
>> +
>> + /* translate the input for clientaddr to nfs_sockaddr */
>> + if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
>> + return 0;
>> +
>> + /* check for IPV4_ANY and IPV6_ANY */
>> + if (nfs_is_inaddr_any(nfs_saddr))
>> + return 1;
>> +
>> + /* check if ip matches local network addresses */
>> + if (!nfs_addr_matches_localips(nfs_saddr)) {
>> + nfs_error(_("%s: supplied clientaddr=%s does not "
>> + "match any existing network addresses"),
>> + progname, addr);
>> + return 0;
>> + } else
>> + return 1;
>> + }
>>
>> nfs_callback_address(sap, salen, my_addr, &my_len);
>>
>>
>
>
> --
> Ben Greear <[email protected]>
> Candela Technologies Inc http://www.candelatech.com
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2018-06-05 16:13:02

by Ben Greear

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] Add check of clientaddr argument

On 06/05/2018 09:05 AM, Olga Kornievskaia wrote:
> On Tue, Jun 5, 2018 at 11:53 AM, Ben Greear <[email protected]> wrote:
>> On 06/05/2018 08:51 AM, Olga Kornievskaia wrote:
>>>
>>> If the user supplies a clientaddr value, it should be either
>>> a special value of either IPv4/IPv6 any address or one of the
>>> machine's network addresses. Otherwise, the use of an arbitrary
>>> value of the clientaddr value is not allowed.
>>
>>
>> Is this finally going to let one bind to a local IP address?
>
> No this patch only checks for the validity of the arguments for the
> callback channel.
>
>> I have patches to do this for years now, would love to get that
>> feature upstream!
>
> Have you posted those patches to the mailing list?

Yes, several times, years ago.

https://www.spinics.net/lists/linux-nfs/msg34811.html

My current trees are here, and the NFS patches are near the bottom of my
patch set. I am not sure we specifically tested this with 4.16 kernel yet, but
we tested it on the 4.13 kernel:

https://github.com/greearb?tab=repositories

The nfs-utils-ct repo has the patches to make nfs-utils support this.

If you or anyone else would like to shepherd these into the kernel
it would be appreciated.

Thanks,
Ben

--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com


2018-06-05 16:49:58

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] Add check of clientaddr argument



> On Jun 5, 2018, at 11:51 AM, Olga Kornievskaia <[email protected]> =
wrote:
>=20
> If the user supplies a clientaddr value,

Suggest instead:

"If an NFS client administrator supplies the clientaddr mount option,"


> it should be either
> a special value of either IPv4/IPv6 any address or one of the
> machine's network addresses. Otherwise, the use of an arbitrary
> value of the clientaddr value is not allowed.

> Signed-off-by: Olga Kornievskaia <[email protected]>
> ---
> utils/mount/network.c | 45 =
+++++++++++++++++++++++++++++++++++++++++++++
> utils/mount/network.h | 2 ++
> utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
> 3 files changed, 72 insertions(+), 3 deletions(-)
>=20
> diff --git a/utils/mount/network.c b/utils/mount/network.c
> index e490399..2a54f82 100644
> --- a/utils/mount/network.c
> +++ b/utils/mount/network.c
> @@ -50,6 +50,8 @@
> #include <rpc/rpc.h>
> #include <rpc/pmap_prot.h>
> #include <rpc/pmap_clnt.h>
> +#include <net/if.h>
> +#include <ifaddrs.h>
>=20
> #include "sockaddr.h"
> #include "xcommon.h"
> @@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options =
*options,
>=20
> return EX_SUCCESS;
> }
> +
> +int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
> +{
> + switch (nfs_saddr->sa_family) {
> + case AF_INET: {
> + if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr =
=3D=3D
> + INADDR_ANY)
> + return 1;
> + }
> + case AF_INET6:
> + if (!memcmp(&((struct sockaddr_in6 =
*)nfs_saddr)->sin6_addr,
> + &in6addr_any, sizeof(in6addr_any)))
> + return 1;
> + }
> + return 0;
> +}
> +
> +int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
> +{
> + struct ifaddrs *myaddrs, *ifa;
> + int found =3D 0;
> +
> + /* acquire exiting network interfaces */
> + if (getifaddrs(&myaddrs) !=3D 0)
> + return 0;
> +
> + /* interate over the available interfaces and check if we
> + * we find a match to the supplied clientaddr value
> + */
> + for (ifa =3D myaddrs; ifa !=3D NULL; ifa =3D ifa->ifa_next) {
> + if (ifa->ifa_addr =3D=3D NULL)
> + continue;
> + if (!(ifa->ifa_flags & IFF_UP))
> + continue;
> + if (!memcmp(ifa->ifa_addr, nfs_saddr,
> + sizeof(struct sockaddr))) {
> + found =3D 1;
> + break;
> + }
> + }
> + freeifaddrs(myaddrs);
> + return found;
> +}

This looks like what we discussed, and it is implemented IMO
correctly. Can you also provide an update to nfs(5)?

After all the dialog and thinking more about it, I'm less convinced
that prohibiting non-local addresses is wise. The client can mount
through a NAT and specify a clientaddr that points to the NAT. That
seems like a correct configuration that would now be prohibited.

However I don't know of any actual deployments like this, so it's
hard to argue convincingly that mount.nfs needs to continue to allow
it.

May I suggest a slight alteration?

- If the value of clientaddr=3D is not a presentation address,
report the problem and fail the mount (no change)

- Allow the value of clientaddr=3D to be an ANY address without
complaint (no change)

- If the value of clientaddr=3D is not a local address, warn but
allow it (rather than failing)

That permits flexibility but also gives a good indication to
administrators who have made an honest mistake.


> diff --git a/utils/mount/network.h b/utils/mount/network.h
> index ecaac33..0fc98ac 100644
> --- a/utils/mount/network.h
> +++ b/utils/mount/network.h
> @@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, =
const socklen_t,
> int clnt_ping(struct sockaddr_in *, const unsigned long,
> const unsigned long, const unsigned int,
> struct sockaddr_in *);
> +int nfs_is_inaddr_any(struct sockaddr *);
> +int nfs_addr_matches_localips(struct sockaddr *);
>=20
> struct mount_options;
>=20
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index d1b0708..e22216f 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct =
sockaddr *sap,
>=20
> /*
> * Called to discover our address and append an appropriate =
'clientaddr=3D'
> - * option to the options string.
> + * option to the options string. If the supplied 'clientaddr=3D' =
value does
> + * not match either IPV4/IPv6 any or a local address, then fail the =
mount.
> *
> * Returns 1 if 'clientaddr=3D' option created successfully or if
> * 'clientaddr=3D' option is already present; otherwise zero.
> @@ -242,8 +243,29 @@ static int nfs_append_clientaddr_option(const =
struct sockaddr *sap,
> struct sockaddr *my_addr =3D &address.sa;
> socklen_t my_len =3D sizeof(address);
>=20
> - if (po_contains(options, "clientaddr") =3D=3D PO_FOUND)
> - return 1;
> + if (po_contains(options, "clientaddr") =3D=3D PO_FOUND) {
> + char *addr =3D po_get(options, "clientaddr");
> + union nfs_sockaddr nfs_address;
> + struct sockaddr *nfs_saddr =3D &nfs_address.sa;
> + socklen_t nfs_salen =3D sizeof(nfs_address);
> +
> + /* translate the input for clientaddr to nfs_sockaddr */
> + if (!nfs_string_to_sockaddr(addr, nfs_saddr, =
&nfs_salen))
> + return 0;
> +
> + /* check for IPV4_ANY and IPV6_ANY */
> + if (nfs_is_inaddr_any(nfs_saddr))
> + return 1;
> +
> + /* check if ip matches local network addresses */
> + if (!nfs_addr_matches_localips(nfs_saddr)) {
> + nfs_error(_("%s: supplied clientaddr=3D%s does =
not "
> + "match any existing network addresses"),
> + progname, addr);
> + return 0;
> + } else
> + return 1;
> + }
>=20
> nfs_callback_address(sap, salen, my_addr, &my_len);
>=20
> --=20
> 1.8.3.1
>=20
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever




2018-06-05 17:28:29

by Olga Kornievskaia

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] Add check of clientaddr argument

On Tue, Jun 5, 2018 at 12:49 PM, Chuck Lever <[email protected]> wrote:
>
>
>> On Jun 5, 2018, at 11:51 AM, Olga Kornievskaia <[email protected]> wrote:
>>
>> If the user supplies a clientaddr value,
>
> Suggest instead:
>
> "If an NFS client administrator supplies the clientaddr mount option,"

Ok.

>> it should be either
>> a special value of either IPv4/IPv6 any address or one of the
>> machine's network addresses. Otherwise, the use of an arbitrary
>> value of the clientaddr value is not allowed.
>
>> Signed-off-by: Olga Kornievskaia <[email protected]>
>> ---
>> utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>> utils/mount/network.h | 2 ++
>> utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
>> 3 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>> index e490399..2a54f82 100644
>> --- a/utils/mount/network.c
>> +++ b/utils/mount/network.c
>> @@ -50,6 +50,8 @@
>> #include <rpc/rpc.h>
>> #include <rpc/pmap_prot.h>
>> #include <rpc/pmap_clnt.h>
>> +#include <net/if.h>
>> +#include <ifaddrs.h>
>>
>> #include "sockaddr.h"
>> #include "xcommon.h"
>> @@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options *options,
>>
>> return EX_SUCCESS;
>> }
>> +
>> +int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
>> +{
>> + switch (nfs_saddr->sa_family) {
>> + case AF_INET: {
>> + if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
>> + INADDR_ANY)
>> + return 1;
>> + }
>> + case AF_INET6:
>> + if (!memcmp(&((struct sockaddr_in6 *)nfs_saddr)->sin6_addr,
>> + &in6addr_any, sizeof(in6addr_any)))
>> + return 1;
>> + }
>> + return 0;
>> +}
>> +
>> +int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
>> +{
>> + struct ifaddrs *myaddrs, *ifa;
>> + int found = 0;
>> +
>> + /* acquire exiting network interfaces */
>> + if (getifaddrs(&myaddrs) != 0)
>> + return 0;
>> +
>> + /* interate over the available interfaces and check if we
>> + * we find a match to the supplied clientaddr value
>> + */
>> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
>> + if (ifa->ifa_addr == NULL)
>> + continue;
>> + if (!(ifa->ifa_flags & IFF_UP))
>> + continue;
>> + if (!memcmp(ifa->ifa_addr, nfs_saddr,
>> + sizeof(struct sockaddr))) {
>> + found = 1;
>> + break;
>> + }
>> + }
>> + freeifaddrs(myaddrs);
>> + return found;
>> +}
>
> This looks like what we discussed, and it is implemented IMO
> correctly. Can you also provide an update to nfs(5)?
>
> After all the dialog and thinking more about it, I'm less convinced
> that prohibiting non-local addresses is wise. The client can mount
> through a NAT and specify a clientaddr that points to the NAT. That
> seems like a correct configuration that would now be prohibited.
>
> However I don't know of any actual deployments like this, so it's
> hard to argue convincingly that mount.nfs needs to continue to allow
> it.
>
> May I suggest a slight alteration?
>
> - If the value of clientaddr= is not a presentation address,
> report the problem and fail the mount (no change)
>
> - Allow the value of clientaddr= to be an ANY address without
> complaint (no change)
>
> - If the value of clientaddr= is not a local address, warn but
> allow it (rather than failing)
>
> That permits flexibility but also gives a good indication to
> administrators who have made an honest mistake.

Given that we are changing the kernel to not use the cl_ipaddr to
construct the client ID, then I'm ok with allowing a none local
address with a warning. But can you tell me how you think specifying a
non local address would actually work? It doesn't make sense to me as
to how the callback would ever reach the appropriate client. When the
server establishes connected back to the NAT router IP, there is
nothing that's listening there. And also NAT router doesn't know where
to send it to.

If we are allowing but warning about the use of the arbitrary IP, do
we need any change to the nfs(5)?

>
>> diff --git a/utils/mount/network.h b/utils/mount/network.h
>> index ecaac33..0fc98ac 100644
>> --- a/utils/mount/network.h
>> +++ b/utils/mount/network.h
>> @@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const socklen_t,
>> int clnt_ping(struct sockaddr_in *, const unsigned long,
>> const unsigned long, const unsigned int,
>> struct sockaddr_in *);
>> +int nfs_is_inaddr_any(struct sockaddr *);
>> +int nfs_addr_matches_localips(struct sockaddr *);
>>
>> struct mount_options;
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index d1b0708..e22216f 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
>>
>> /*
>> * Called to discover our address and append an appropriate 'clientaddr='
>> - * option to the options string.
>> + * option to the options string. If the supplied 'clientaddr=' value does
>> + * not match either IPV4/IPv6 any or a local address, then fail the mount.
>> *
>> * Returns 1 if 'clientaddr=' option created successfully or if
>> * 'clientaddr=' option is already present; otherwise zero.
>> @@ -242,8 +243,29 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
>> struct sockaddr *my_addr = &address.sa;
>> socklen_t my_len = sizeof(address);
>>
>> - if (po_contains(options, "clientaddr") == PO_FOUND)
>> - return 1;
>> + if (po_contains(options, "clientaddr") == PO_FOUND) {
>> + char *addr = po_get(options, "clientaddr");
>> + union nfs_sockaddr nfs_address;
>> + struct sockaddr *nfs_saddr = &nfs_address.sa;
>> + socklen_t nfs_salen = sizeof(nfs_address);
>> +
>> + /* translate the input for clientaddr to nfs_sockaddr */
>> + if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
>> + return 0;
>> +
>> + /* check for IPV4_ANY and IPV6_ANY */
>> + if (nfs_is_inaddr_any(nfs_saddr))
>> + return 1;
>> +
>> + /* check if ip matches local network addresses */
>> + if (!nfs_addr_matches_localips(nfs_saddr)) {
>> + nfs_error(_("%s: supplied clientaddr=%s does not "
>> + "match any existing network addresses"),
>> + progname, addr);
>> + return 0;
>> + } else
>> + return 1;
>> + }
>>
>> nfs_callback_address(sap, salen, my_addr, &my_len);
>>
>> --
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Chuck Lever
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2018-06-05 17:45:01

by Chuck Lever III

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] Add check of clientaddr argument



> On Jun 5, 2018, at 1:28 PM, Olga Kornievskaia <[email protected]> wrote:
>=20
> On Tue, Jun 5, 2018 at 12:49 PM, Chuck Lever <[email protected]> =
wrote:
>>=20
>>=20
>>> On Jun 5, 2018, at 11:51 AM, Olga Kornievskaia <[email protected]> =
wrote:
>>>=20
>>> If the user supplies a clientaddr value,
>>=20
>> Suggest instead:
>>=20
>> "If an NFS client administrator supplies the clientaddr mount =
option,"
>=20
> Ok.
>=20
>>> it should be either
>>> a special value of either IPv4/IPv6 any address or one of the
>>> machine's network addresses. Otherwise, the use of an arbitrary
>>> value of the clientaddr value is not allowed.
>>=20
>>> Signed-off-by: Olga Kornievskaia <[email protected]>
>>> ---
>>> utils/mount/network.c | 45 =
+++++++++++++++++++++++++++++++++++++++++++++
>>> utils/mount/network.h | 2 ++
>>> utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
>>> 3 files changed, 72 insertions(+), 3 deletions(-)
>>>=20
>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>> index e490399..2a54f82 100644
>>> --- a/utils/mount/network.c
>>> +++ b/utils/mount/network.c
>>> @@ -50,6 +50,8 @@
>>> #include <rpc/rpc.h>
>>> #include <rpc/pmap_prot.h>
>>> #include <rpc/pmap_clnt.h>
>>> +#include <net/if.h>
>>> +#include <ifaddrs.h>
>>>=20
>>> #include "sockaddr.h"
>>> #include "xcommon.h"
>>> @@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options =
*options,
>>>=20
>>> return EX_SUCCESS;
>>> }
>>> +
>>> +int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
>>> +{
>>> + switch (nfs_saddr->sa_family) {
>>> + case AF_INET: {
>>> + if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr =
=3D=3D
>>> + INADDR_ANY)
>>> + return 1;
>>> + }
>>> + case AF_INET6:
>>> + if (!memcmp(&((struct sockaddr_in6 =
*)nfs_saddr)->sin6_addr,
>>> + &in6addr_any, sizeof(in6addr_any)))
>>> + return 1;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
>>> +{
>>> + struct ifaddrs *myaddrs, *ifa;
>>> + int found =3D 0;
>>> +
>>> + /* acquire exiting network interfaces */
>>> + if (getifaddrs(&myaddrs) !=3D 0)
>>> + return 0;
>>> +
>>> + /* interate over the available interfaces and check if we
>>> + * we find a match to the supplied clientaddr value
>>> + */
>>> + for (ifa =3D myaddrs; ifa !=3D NULL; ifa =3D ifa->ifa_next) {
>>> + if (ifa->ifa_addr =3D=3D NULL)
>>> + continue;
>>> + if (!(ifa->ifa_flags & IFF_UP))
>>> + continue;
>>> + if (!memcmp(ifa->ifa_addr, nfs_saddr,
>>> + sizeof(struct sockaddr))) {
>>> + found =3D 1;
>>> + break;
>>> + }
>>> + }
>>> + freeifaddrs(myaddrs);
>>> + return found;
>>> +}
>>=20
>> This looks like what we discussed, and it is implemented IMO
>> correctly.

While I was watching TV and eating lunch, it occurred to me
that mount.nfs could pass the clientaddr value to bind(2).
bind(2) should work for an ANY address or a local address, but
would fail with a non-local address, unless I'm confused. Just
a random thought.


>> Can you also provide an update to nfs(5)?

>> After all the dialog and thinking more about it, I'm less convinced
>> that prohibiting non-local addresses is wise. The client can mount
>> through a NAT and specify a clientaddr that points to the NAT. That
>> seems like a correct configuration that would now be prohibited.
>>=20
>> However I don't know of any actual deployments like this, so it's
>> hard to argue convincingly that mount.nfs needs to continue to allow
>> it.
>>=20
>> May I suggest a slight alteration?
>>=20
>> - If the value of clientaddr=3D is not a presentation address,
>> report the problem and fail the mount (no change)
>>=20
>> - Allow the value of clientaddr=3D to be an ANY address without
>> complaint (no change)
>>=20
>> - If the value of clientaddr=3D is not a local address, warn but
>> allow it (rather than failing)
>>=20
>> That permits flexibility but also gives a good indication to
>> administrators who have made an honest mistake.
>=20
> Given that we are changing the kernel to not use the cl_ipaddr to
> construct the client ID, then I'm ok with allowing a none local
> address with a warning. But can you tell me how you think specifying a
> non local address would actually work? It doesn't make sense to me as
> to how the callback would ever reach the appropriate client. When the
> server establishes connected back to the NAT router IP, there is
> nothing that's listening there. And also NAT router doesn't know where
> to send it to.

How it might work:

- Each client would have it's own callback service running on
a distinct port

- The NAT would need to be programmed to route the callback
connection to the correct client, by port


> If we are allowing but warning about the use of the arbitrary IP, do
> we need any change to the nfs(5)?

Hm...

You mentioned nfs(5) doesn't document the clientaddr=3DANY trick.
If we want to leave that undocumented (say, to try to implement
a more pleasant administrative interface to disable NFSv4.0
callback) then I suppose we could leave nfs(5) alone.


>>> diff --git a/utils/mount/network.h b/utils/mount/network.h
>>> index ecaac33..0fc98ac 100644
>>> --- a/utils/mount/network.h
>>> +++ b/utils/mount/network.h
>>> @@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, =
const socklen_t,
>>> int clnt_ping(struct sockaddr_in *, const unsigned long,
>>> const unsigned long, const unsigned int,
>>> struct sockaddr_in *);
>>> +int nfs_is_inaddr_any(struct sockaddr *);
>>> +int nfs_addr_matches_localips(struct sockaddr *);
>>>=20
>>> struct mount_options;
>>>=20
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index d1b0708..e22216f 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct =
sockaddr *sap,
>>>=20
>>> /*
>>> * Called to discover our address and append an appropriate =
'clientaddr=3D'
>>> - * option to the options string.
>>> + * option to the options string. If the supplied 'clientaddr=3D' =
value does
>>> + * not match either IPV4/IPv6 any or a local address, then fail the =
mount.
>>> *
>>> * Returns 1 if 'clientaddr=3D' option created successfully or if
>>> * 'clientaddr=3D' option is already present; otherwise zero.
>>> @@ -242,8 +243,29 @@ static int nfs_append_clientaddr_option(const =
struct sockaddr *sap,
>>> struct sockaddr *my_addr =3D &address.sa;
>>> socklen_t my_len =3D sizeof(address);
>>>=20
>>> - if (po_contains(options, "clientaddr") =3D=3D PO_FOUND)
>>> - return 1;
>>> + if (po_contains(options, "clientaddr") =3D=3D PO_FOUND) {
>>> + char *addr =3D po_get(options, "clientaddr");
>>> + union nfs_sockaddr nfs_address;
>>> + struct sockaddr *nfs_saddr =3D &nfs_address.sa;
>>> + socklen_t nfs_salen =3D sizeof(nfs_address);
>>> +
>>> + /* translate the input for clientaddr to nfs_sockaddr =
*/
>>> + if (!nfs_string_to_sockaddr(addr, nfs_saddr, =
&nfs_salen))
>>> + return 0;
>>> +
>>> + /* check for IPV4_ANY and IPV6_ANY */
>>> + if (nfs_is_inaddr_any(nfs_saddr))
>>> + return 1;
>>> +
>>> + /* check if ip matches local network addresses */
>>> + if (!nfs_addr_matches_localips(nfs_saddr)) {
>>> + nfs_error(_("%s: supplied clientaddr=3D%s does =
not "
>>> + "match any existing network =
addresses"),
>>> + progname, addr);
>>> + return 0;
>>> + } else
>>> + return 1;
>>> + }
>>>=20
>>> nfs_callback_address(sap, salen, my_addr, &my_len);
>>>=20
>>> --
>>> 1.8.3.1
>>>=20
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Chuck Lever




2018-06-05 19:05:28

by Olga Kornievskaia

[permalink] [raw]
Subject: Re: [PATCH v2 1/1] Add check of clientaddr argument

On Tue, Jun 5, 2018 at 1:44 PM, Chuck Lever <[email protected]> wrote:
>
>
>> On Jun 5, 2018, at 1:28 PM, Olga Kornievskaia <[email protected]> wrote:
>>
>> On Tue, Jun 5, 2018 at 12:49 PM, Chuck Lever <[email protected]> wrote:
>>>
>>>
>>>> On Jun 5, 2018, at 11:51 AM, Olga Kornievskaia <[email protected]> wrote:
>>>>
>>>> If the user supplies a clientaddr value,
>>>
>>> Suggest instead:
>>>
>>> "If an NFS client administrator supplies the clientaddr mount option,"
>>
>> Ok.
>>
>>>> it should be either
>>>> a special value of either IPv4/IPv6 any address or one of the
>>>> machine's network addresses. Otherwise, the use of an arbitrary
>>>> value of the clientaddr value is not allowed.
>>>
>>>> Signed-off-by: Olga Kornievskaia <[email protected]>
>>>> ---
>>>> utils/mount/network.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>>> utils/mount/network.h | 2 ++
>>>> utils/mount/stropts.c | 28 +++++++++++++++++++++++++---
>>>> 3 files changed, 72 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/utils/mount/network.c b/utils/mount/network.c
>>>> index e490399..2a54f82 100644
>>>> --- a/utils/mount/network.c
>>>> +++ b/utils/mount/network.c
>>>> @@ -50,6 +50,8 @@
>>>> #include <rpc/rpc.h>
>>>> #include <rpc/pmap_prot.h>
>>>> #include <rpc/pmap_clnt.h>
>>>> +#include <net/if.h>
>>>> +#include <ifaddrs.h>
>>>>
>>>> #include "sockaddr.h"
>>>> #include "xcommon.h"
>>>> @@ -1759,3 +1761,46 @@ int nfs_umount_do_umnt(struct mount_options *options,
>>>>
>>>> return EX_SUCCESS;
>>>> }
>>>> +
>>>> +int nfs_is_inaddr_any(struct sockaddr *nfs_saddr)
>>>> +{
>>>> + switch (nfs_saddr->sa_family) {
>>>> + case AF_INET: {
>>>> + if (((struct sockaddr_in *)nfs_saddr)->sin_addr.s_addr ==
>>>> + INADDR_ANY)
>>>> + return 1;
>>>> + }
>>>> + case AF_INET6:
>>>> + if (!memcmp(&((struct sockaddr_in6 *)nfs_saddr)->sin6_addr,
>>>> + &in6addr_any, sizeof(in6addr_any)))
>>>> + return 1;
>>>> + }
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int nfs_addr_matches_localips(struct sockaddr *nfs_saddr)
>>>> +{
>>>> + struct ifaddrs *myaddrs, *ifa;
>>>> + int found = 0;
>>>> +
>>>> + /* acquire exiting network interfaces */
>>>> + if (getifaddrs(&myaddrs) != 0)
>>>> + return 0;
>>>> +
>>>> + /* interate over the available interfaces and check if we
>>>> + * we find a match to the supplied clientaddr value
>>>> + */
>>>> + for (ifa = myaddrs; ifa != NULL; ifa = ifa->ifa_next) {
>>>> + if (ifa->ifa_addr == NULL)
>>>> + continue;
>>>> + if (!(ifa->ifa_flags & IFF_UP))
>>>> + continue;
>>>> + if (!memcmp(ifa->ifa_addr, nfs_saddr,
>>>> + sizeof(struct sockaddr))) {
>>>> + found = 1;
>>>> + break;
>>>> + }
>>>> + }
>>>> + freeifaddrs(myaddrs);
>>>> + return found;
>>>> +}
>>>
>>> This looks like what we discussed, and it is implemented IMO
>>> correctly.
>
> While I was watching TV and eating lunch, it occurred to me
> that mount.nfs could pass the clientaddr value to bind(2).
> bind(2) should work for an ANY address or a local address, but
> would fail with a non-local address, unless I'm confused. Just
> a random thought.

Now I'm confused. Where would we be calling bind? mount.nfs does not
call bind. kernel calls svc_create_xprt() to create the callback
server and the only thing it takes is nfs_callback_set_tcpport (which
is a module parameter and I guess this is what you were thinking a
client could use to start on a dedicated port correct. Caz otherwise I
think the port is gotten at random)? I don't that the value of
clientaddr is at all used in creating the callback server. Or is this
a comment to what Ben Greear is proposing (where he wants to bind()
the client to the specified source address. though in his case I think
it's a fore channel).

I guess since we don't call bind then specifying a non-local address
can currently work and somebody could do the NAT-ed setup. If we did
call bind, then it wouldn't work and the check should be changed from
warning to failing.

>>> Can you also provide an update to nfs(5)?
>
>>> After all the dialog and thinking more about it, I'm less convinced
>>> that prohibiting non-local addresses is wise. The client can mount
>>> through a NAT and specify a clientaddr that points to the NAT. That
>>> seems like a correct configuration that would now be prohibited.
>>>
>>> However I don't know of any actual deployments like this, so it's
>>> hard to argue convincingly that mount.nfs needs to continue to allow
>>> it.
>>>
>>> May I suggest a slight alteration?
>>>
>>> - If the value of clientaddr= is not a presentation address,
>>> report the problem and fail the mount (no change)
>>>
>>> - Allow the value of clientaddr= to be an ANY address without
>>> complaint (no change)
>>>
>>> - If the value of clientaddr= is not a local address, warn but
>>> allow it (rather than failing)
>>>
>>> That permits flexibility but also gives a good indication to
>>> administrators who have made an honest mistake.
>>
>> Given that we are changing the kernel to not use the cl_ipaddr to
>> construct the client ID, then I'm ok with allowing a none local
>> address with a warning. But can you tell me how you think specifying a
>> non local address would actually work? It doesn't make sense to me as
>> to how the callback would ever reach the appropriate client. When the
>> server establishes connected back to the NAT router IP, there is
>> nothing that's listening there. And also NAT router doesn't know where
>> to send it to.
>
> How it might work:
>
> - Each client would have it's own callback service running on
> a distinct port

I'm assuming this is done via "callback_tcpport" module parameter then.

> - The NAT would need to be programmed to route the callback
> connection to the correct client, by port

Complicated :)

>> If we are allowing but warning about the use of the arbitrary IP, do
>> we need any change to the nfs(5)?
>
> Hm...
>
> You mentioned nfs(5) doesn't document the clientaddr=ANY trick.

Got it. Yes I think adding something to the man page would be useful.
Let me figure out how to add to man pages. Should it be a separate
patch or a part of this patch?

> If we want to leave that undocumented (say, to try to implement
> a more pleasant administrative interface to disable NFSv4.0
> callback) then I suppose we could leave nfs(5) alone.

We could then change it again? I don't know...

>>>> diff --git a/utils/mount/network.h b/utils/mount/network.h
>>>> index ecaac33..0fc98ac 100644
>>>> --- a/utils/mount/network.h
>>>> +++ b/utils/mount/network.h
>>>> @@ -54,6 +54,8 @@ int nfs_callback_address(const struct sockaddr *, const socklen_t,
>>>> int clnt_ping(struct sockaddr_in *, const unsigned long,
>>>> const unsigned long, const unsigned int,
>>>> struct sockaddr_in *);
>>>> +int nfs_is_inaddr_any(struct sockaddr *);
>>>> +int nfs_addr_matches_localips(struct sockaddr *);
>>>>
>>>> struct mount_options;
>>>>
>>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>>> index d1b0708..e22216f 100644
>>>> --- a/utils/mount/stropts.c
>>>> +++ b/utils/mount/stropts.c
>>>> @@ -229,7 +229,8 @@ static int nfs_append_addr_option(const struct sockaddr *sap,
>>>>
>>>> /*
>>>> * Called to discover our address and append an appropriate 'clientaddr='
>>>> - * option to the options string.
>>>> + * option to the options string. If the supplied 'clientaddr=' value does
>>>> + * not match either IPV4/IPv6 any or a local address, then fail the mount.
>>>> *
>>>> * Returns 1 if 'clientaddr=' option created successfully or if
>>>> * 'clientaddr=' option is already present; otherwise zero.
>>>> @@ -242,8 +243,29 @@ static int nfs_append_clientaddr_option(const struct sockaddr *sap,
>>>> struct sockaddr *my_addr = &address.sa;
>>>> socklen_t my_len = sizeof(address);
>>>>
>>>> - if (po_contains(options, "clientaddr") == PO_FOUND)
>>>> - return 1;
>>>> + if (po_contains(options, "clientaddr") == PO_FOUND) {
>>>> + char *addr = po_get(options, "clientaddr");
>>>> + union nfs_sockaddr nfs_address;
>>>> + struct sockaddr *nfs_saddr = &nfs_address.sa;
>>>> + socklen_t nfs_salen = sizeof(nfs_address);
>>>> +
>>>> + /* translate the input for clientaddr to nfs_sockaddr */
>>>> + if (!nfs_string_to_sockaddr(addr, nfs_saddr, &nfs_salen))
>>>> + return 0;
>>>> +
>>>> + /* check for IPV4_ANY and IPV6_ANY */
>>>> + if (nfs_is_inaddr_any(nfs_saddr))
>>>> + return 1;
>>>> +
>>>> + /* check if ip matches local network addresses */
>>>> + if (!nfs_addr_matches_localips(nfs_saddr)) {
>>>> + nfs_error(_("%s: supplied clientaddr=%s does not "
>>>> + "match any existing network addresses"),
>>>> + progname, addr);
>>>> + return 0;
>>>> + } else
>>>> + return 1;
>>>> + }
>>>>
>>>> nfs_callback_address(sap, salen, my_addr, &my_len);
>>>>
>>>> --
>>>> 1.8.3.1
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>>> the body of a message to [email protected]
>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>> --
>>> Chuck Lever
>>>
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Chuck Lever
>
>
>