LinuxLists.cc - [PATCH 1/2] tcp: md5: add an address prefix for key lookup

2017-06-07 01:01:59

Subject: [PATCH 1/2] tcp: md5: add an address prefix for key lookup

This allows the keys used for TCP MD5 signature to be used for whole
range of addresses, specified with a prefix length, instead of only one
address as it currently is.

Signed-off-by: Bob Gilligan <[email protected]>
Signed-off-by: Eric Mowat <[email protected]>
Signed-off-by: Ivan Delalande <[email protected]>
---
include/net/tcp.h | 6 +++--
net/ipv4/tcp_ipv4.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++-------
net/ipv6/tcp_ipv6.c | 12 ++++++----
3 files changed, 70 insertions(+), 16 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 38a7427ae902..2b68023ab095 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1395,6 +1395,7 @@ struct tcp_md5sig_key {
u8 keylen;
u8 family; /* AF_INET or AF_INET6 */
union tcp_md5_addr addr;
+ u8 prefixlen;
u8 key[TCP_MD5SIG_MAXKEYLEN];
struct rcu_head rcu;
};
@@ -1438,9 +1439,10 @@ struct tcp_md5sig_pool {
int tcp_v4_md5_hash_skb(char *md5_hash, const struct tcp_md5sig_key *key,
const struct sock *sk, const struct sk_buff *skb);
int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
- int family, const u8 *newkey, u8 newkeylen, gfp_t gfp);
+ int family, u8 prefixlen, const u8 *newkey, u8 newkeylen,
+ gfp_t gfp);
int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr,
- int family);
+ int family, u8 prefixlen);
struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk,
const struct sock *addr_sk);

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 5ab2aac5ca19..51ca3bd5a8a3 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -80,6 +80,7 @@
#include <linux/stddef.h>
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
+#include <linux/inetdevice.h>

#include <crypto/hash.h>
#include <linux/scatterlist.h>
@@ -906,6 +907,9 @@ struct tcp_md5sig_key *tcp_md5_do_lookup(const struct sock *sk,
struct tcp_md5sig_key *key;
unsigned int size = sizeof(struct in_addr);
const struct tcp_md5sig_info *md5sig;
+ __be32 mask;
+ struct tcp_md5sig_key *best_match = NULL;
+ bool match;

/* caller either holds rcu_read_lock() or socket lock */
md5sig = rcu_dereference_check(tp->md5sig_info,
@@ -919,12 +923,55 @@ struct tcp_md5sig_key *tcp_md5_do_lookup(const struct sock *sk,
hlist_for_each_entry_rcu(key, &md5sig->head, node) {
if (key->family != family)
continue;
- if (!memcmp(&key->addr, addr, size))
+
+ if (family == AF_INET) {
+ mask = inet_make_mask(key->prefixlen);
+ match = (key->addr.a4.s_addr & mask) ==
+ (addr->a4.s_addr & mask);
+#if IS_ENABLED(CONFIG_IPV6)
+ } else if (family == AF_INET6) {
+ match = ipv6_prefix_equal(&key->addr.a6, &addr->a6,
+ key->prefixlen);
+#endif
+ } else {
+ match = false;
+ }
+
+ if (match && (!best_match ||
+ key->prefixlen > best_match->prefixlen))
+ best_match = key;
+ }
+ return best_match;
+}
+EXPORT_SYMBOL(tcp_md5_do_lookup);
+
+struct tcp_md5sig_key *tcp_md5_do_lookup_exact(const struct sock *sk,
+ const union tcp_md5_addr *addr,
+ int family, u8 prefixlen)
+{
+ const struct tcp_sock *tp = tcp_sk(sk);
+ struct tcp_md5sig_key *key;
+ unsigned int size = sizeof(struct in_addr);
+ const struct tcp_md5sig_info *md5sig;
+
+ /* caller either holds rcu_read_lock() or socket lock */
+ md5sig = rcu_dereference_check(tp->md5sig_info,
+ lockdep_sock_is_held(sk));
+ if (!md5sig)
+ return NULL;
+#if IS_ENABLED(CONFIG_IPV6)
+ if (family == AF_INET6)
+ size = sizeof(struct in6_addr);
+#endif
+ hlist_for_each_entry_rcu(key, &md5sig->head, node) {
+ if (key->family != family)
+ continue;
+ if (!memcmp(&key->addr, addr, size) &&
+ key->prefixlen == prefixlen)
return key;
}
return NULL;
}
-EXPORT_SYMBOL(tcp_md5_do_lookup);

struct tcp_md5sig_key *tcp_v4_md5_lookup(const struct sock *sk,
const struct sock *addr_sk)
@@ -938,14 +985,15 @@ EXPORT_SYMBOL(tcp_v4_md5_lookup);

/* This can be called on a newly created socket, from other files */
int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
- int family, const u8 *newkey, u8 newkeylen, gfp_t gfp)
+ int family, u8 prefixlen, const u8 *newkey, u8 newkeylen,
+ gfp_t gfp)
{
/* Add Key to the list */
struct tcp_md5sig_key *key;
struct tcp_sock *tp = tcp_sk(sk);
struct tcp_md5sig_info *md5sig;

- key = tcp_md5_do_lookup(sk, addr, family);
+ key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen);
if (key) {
/* Pre-existing entry - just update that one. */
memcpy(key->key, newkey, newkeylen);
@@ -976,6 +1024,7 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
memcpy(key->key, newkey, newkeylen);
key->keylen = newkeylen;
key->family = family;
+ key->prefixlen = prefixlen;
memcpy(&key->addr, addr,
(family == AF_INET6) ? sizeof(struct in6_addr) :
sizeof(struct in_addr));
@@ -984,11 +1033,12 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr,
}
EXPORT_SYMBOL(tcp_md5_do_add);

-int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, int family)
+int tcp_md5_do_del(struct sock *sk, const union tcp_md5_addr *addr, int family,
+ u8 prefixlen)
{
struct tcp_md5sig_key *key;

- key = tcp_md5_do_lookup(sk, addr, family);
+ key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen);
if (!key)
return -ENOENT;
hlist_del_rcu(&key->node);
@@ -1031,13 +1081,13 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,

if (!cmd.tcpm_keylen)
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr,
- AF_INET);
+ AF_INET, 32);

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
return -EINVAL;

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr,
- AF_INET, cmd.tcpm_key, cmd.tcpm_keylen,
+ AF_INET, 32, cmd.tcpm_key, cmd.tcpm_keylen,
GFP_KERNEL);
}

@@ -1340,7 +1390,7 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
* across. Shucks.
*/
tcp_md5_do_add(newsk, (union tcp_md5_addr *)&newinet->inet_daddr,
- AF_INET, key->key, key->keylen, GFP_ATOMIC);
+ AF_INET, 32, key->key, key->keylen, GFP_ATOMIC);
sk_nocaps_add(newsk, NETIF_F_GSO_MASK);
}
#endif
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 7a8237acd210..5cf19dab60aa 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -532,9 +532,9 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
if (!cmd.tcpm_keylen) {
if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
- AF_INET);
+ AF_INET, 32);
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
- AF_INET6);
+ AF_INET6, 128);
}

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
@@ -542,10 +542,12 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,

if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
- AF_INET, cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
+ AF_INET, 32, cmd.tcpm_key,
+ cmd.tcpm_keylen, GFP_KERNEL);

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
- AF_INET6, cmd.tcpm_key, cmd.tcpm_keylen, GFP_KERNEL);
+ AF_INET6, 128, cmd.tcpm_key, cmd.tcpm_keylen,
+ GFP_KERNEL);
}

static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp,
@@ -1183,7 +1185,7 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
* across. Shucks.
*/
tcp_md5_do_add(newsk, (union tcp_md5_addr *)&newsk->sk_v6_daddr,
- AF_INET6, key->key, key->keylen,
+ AF_INET6, 128, key->key, key->keylen,
sk_gfp_mask(sk, GFP_ATOMIC));
}
#endif
--
2.13.0

2017-06-07 01:01:58

by Ivan Delalande

[permalink] [raw]

Subject: [PATCH 2/2] tcp: md5: add fields to the tcp_md5sig struct to set a key address prefix

Replace padding in the socket option structure tcp_md5sig with a new
flag field and address prefix length so it can be specified when
configuring a new key with the TCP_MD5SIG socket option.

Signed-off-by: Bob Gilligan <[email protected]>
Signed-off-by: Eric Mowat <[email protected]>
Signed-off-by: Ivan Delalande <[email protected]>
---
include/uapi/linux/tcp.h | 6 +++++-
net/ipv4/tcp_ipv4.c | 13 +++++++++++--
net/ipv6/tcp_ipv6.c | 20 +++++++++++++++-----
3 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 38a2b07afdff..52ac30aa0652 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -234,9 +234,13 @@ enum {
/* for TCP_MD5SIG socket option */
#define TCP_MD5SIG_MAXKEYLEN 80

+/* tcp_md5sig flags */
+#define TCP_MD5SIG_FLAG_PREFIX 1 /* address prefix length */
+
struct tcp_md5sig {
struct __kernel_sockaddr_storage tcpm_addr; /* address associated */
- __u16 __tcpm_pad1; /* zero */
+ __u8 tcpm_flags; /* flags */
+ __u8 tcpm_prefixlen; /* address prefix */
__u16 tcpm_keylen; /* key length */
__u32 __tcpm_pad2; /* zero */
__u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN]; /* key (binary) */
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 51ca3bd5a8a3..2b1bb67b3388 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1069,6 +1069,7 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
{
struct tcp_md5sig cmd;
struct sockaddr_in *sin = (struct sockaddr_in *)&cmd.tcpm_addr;
+ u8 prefixlen;

if (optlen < sizeof(cmd))
return -EINVAL;
@@ -1079,15 +1080,23 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
if (sin->sin_family != AF_INET)
return -EINVAL;

+ if (cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
+ prefixlen = cmd.tcpm_prefixlen;
+ if (prefixlen > 32)
+ return -EINVAL;
+ } else {
+ prefixlen = 32;
+ }
+
if (!cmd.tcpm_keylen)
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr,
- AF_INET, 32);
+ AF_INET, prefixlen);

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
return -EINVAL;

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr,
- AF_INET, 32, cmd.tcpm_key, cmd.tcpm_keylen,
+ AF_INET, prefixlen, cmd.tcpm_key, cmd.tcpm_keylen,
GFP_KERNEL);
}

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 5cf19dab60aa..f293fc69e88b 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -519,6 +519,7 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
{
struct tcp_md5sig cmd;
struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&cmd.tcpm_addr;
+ u8 prefixlen;

if (optlen < sizeof(cmd))
return -EINVAL;
@@ -529,12 +530,21 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
if (sin6->sin6_family != AF_INET6)
return -EINVAL;

+ if (cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
+ prefixlen = cmd.tcpm_prefixlen;
+ if (prefixlen > 128 || (ipv6_addr_v4mapped(&sin6->sin6_addr) &&
+ prefixlen > 32))
+ return -EINVAL;
+ } else {
+ prefixlen = ipv6_addr_v4mapped(&sin6->sin6_addr) ? 32 : 128;
+ }
+
if (!cmd.tcpm_keylen) {
if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
- AF_INET, 32);
+ AF_INET, prefixlen);
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
- AF_INET6, 128);
+ AF_INET6, prefixlen);
}

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
@@ -542,12 +552,12 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,

if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
- AF_INET, 32, cmd.tcpm_key,
+ AF_INET, prefixlen, cmd.tcpm_key,
cmd.tcpm_keylen, GFP_KERNEL);

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
- AF_INET6, 128, cmd.tcpm_key, cmd.tcpm_keylen,
- GFP_KERNEL);
+ AF_INET6, prefixlen, cmd.tcpm_key,
+ cmd.tcpm_keylen, GFP_KERNEL);
}

static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp,
--
2.13.0

2017-06-07 04:08:27

by Eric Dumazet

[permalink] [raw]

Subject: Re: [PATCH 2/2] tcp: md5: add fields to the tcp_md5sig struct to set a key address prefix

On Tue, 2017-06-06 at 17:54 -0700, Ivan Delalande wrote:
> Replace padding in the socket option structure tcp_md5sig with a new
> flag field and address prefix length so it can be specified when
> configuring a new key with the TCP_MD5SIG socket option.
>
> Signed-off-by: Bob Gilligan <[email protected]>
> Signed-off-by: Eric Mowat <[email protected]>
> Signed-off-by: Ivan Delalande <[email protected]>
> ---
> include/uapi/linux/tcp.h | 6 +++++-
> net/ipv4/tcp_ipv4.c | 13 +++++++++++--
> net/ipv6/tcp_ipv6.c | 20 +++++++++++++++-----
> 3 files changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> index 38a2b07afdff..52ac30aa0652 100644
> --- a/include/uapi/linux/tcp.h
> +++ b/include/uapi/linux/tcp.h
> @@ -234,9 +234,13 @@ enum {
> /* for TCP_MD5SIG socket option */
> #define TCP_MD5SIG_MAXKEYLEN 80
>
> +/* tcp_md5sig flags */
> +#define TCP_MD5SIG_FLAG_PREFIX 1 /* address prefix length */
> +
> struct tcp_md5sig {
> struct __kernel_sockaddr_storage tcpm_addr; /* address associated */
> - __u16 __tcpm_pad1; /* zero */
> + __u8 tcpm_flags; /* flags */
> + __u8 tcpm_prefixlen; /* address prefix */
> __u16 tcpm_keylen; /* key length */
> __u32 __tcpm_pad2; /* zero */
> __u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN]; /* key (binary) */
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 51ca3bd5a8a3..2b1bb67b3388 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1069,6 +1069,7 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
> {
> struct tcp_md5sig cmd;
> struct sockaddr_in *sin = (struct sockaddr_in *)&cmd.tcpm_addr;
> + u8 prefixlen;
>
> if (optlen < sizeof(cmd))
> return -EINVAL;
> @@ -1079,15 +1080,23 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
> if (sin->sin_family != AF_INET)
> return -EINVAL;
>
> + if (cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
> + prefixlen = cmd.tcpm_prefixlen;
> + if (prefixlen > 32)
> + return -EINVAL;
> + } else {
> + prefixlen = 32;
> + }

This will break some applications that maybe did not clear the
__tcpm_pad1 field ?

You need to find another way to maintain compatibility with old
applications.

2017-06-07 06:13:25

by Ivan Delalande

[permalink] [raw]

Subject: Re: [PATCH 2/2] tcp: md5: add fields to the tcp_md5sig struct to set a key address prefix

On Tue, Jun 06, 2017 at 09:08:22PM -0700, Eric Dumazet wrote:
> On Tue, 2017-06-06 at 17:54 -0700, Ivan Delalande wrote:
>> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
>> index 38a2b07afdff..52ac30aa0652 100644
>> --- a/include/uapi/linux/tcp.h
>> +++ b/include/uapi/linux/tcp.h
>> @@ -234,9 +234,13 @@ enum {
>> /* for TCP_MD5SIG socket option */
>> #define TCP_MD5SIG_MAXKEYLEN 80
>>
>> +/* tcp_md5sig flags */
>> +#define TCP_MD5SIG_FLAG_PREFIX 1 /* address prefix length */
>> +
>> struct tcp_md5sig {
>> struct __kernel_sockaddr_storage tcpm_addr; /* address associated */
>> - __u16 __tcpm_pad1; /* zero */
>> + __u8 tcpm_flags; /* flags */
>> + __u8 tcpm_prefixlen; /* address prefix */
>> __u16 tcpm_keylen; /* key length */
>> __u32 __tcpm_pad2; /* zero */
>> __u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN]; /* key (binary) */
>
> This will break some applications that maybe did not clear the
> __tcpm_pad1 field ?
>
>
> You need to find another way to maintain compatibility with old
> applications.

All right, I thought this was acceptable after seeing a few examples of
this in commits extending other structures in uapi, but the context and
use were probably different for those.

We had another version of this patch which steals a bit from tcpm_keylen
to use as a flag for this feature specifically and with the prefixlen at
the same place as this patch. So when the flag is set we know we can
safely interpret this part of the padding field as a prefix as all valid
calls from older user programs should not have a key length greater than
80 bytes.

Would this be better? Programs compiled with the new headers could break
on older kernels if they don't check the version, I don't know if that's
a concern.

Or should we just add these two new fields at the end of tcp_md5sig and
use them only if the value of optlen in the parse function called from
setsockopt is large enough?

Thank you,
--
Ivan Delalande
Arista Networks

2017-06-07 12:51:28

by Eric Dumazet

[permalink] [raw]

Subject: Re: [PATCH 2/2] tcp: md5: add fields to the tcp_md5sig struct to set a key address prefix

On Wed, 2017-06-07 at 08:13 +0200, Ivan Delalande wrote:
> On Tue, Jun 06, 2017 at 09:08:22PM -0700, Eric Dumazet wrote:
> > On Tue, 2017-06-06 at 17:54 -0700, Ivan Delalande wrote:
> >> diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
> >> index 38a2b07afdff..52ac30aa0652 100644
> >> --- a/include/uapi/linux/tcp.h
> >> +++ b/include/uapi/linux/tcp.h
> >> @@ -234,9 +234,13 @@ enum {
> >> /* for TCP_MD5SIG socket option */
> >> #define TCP_MD5SIG_MAXKEYLEN 80
> >>
> >> +/* tcp_md5sig flags */
> >> +#define TCP_MD5SIG_FLAG_PREFIX 1 /* address prefix length */
> >> +
> >> struct tcp_md5sig {
> >> struct __kernel_sockaddr_storage tcpm_addr; /* address associated */
> >> - __u16 __tcpm_pad1; /* zero */
> >> + __u8 tcpm_flags; /* flags */
> >> + __u8 tcpm_prefixlen; /* address prefix */
> >> __u16 tcpm_keylen; /* key length */
> >> __u32 __tcpm_pad2; /* zero */
> >> __u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN]; /* key (binary) */
> >
> > This will break some applications that maybe did not clear the
> > __tcpm_pad1 field ?
> >
> >
> > You need to find another way to maintain compatibility with old
> > applications.
>
> All right, I thought this was acceptable after seeing a few examples of
> this in commits extending other structures in uapi, but the context and
> use were probably different for those.
>
> We had another version of this patch which steals a bit from tcpm_keylen
> to use as a flag for this feature specifically and with the prefixlen at
> the same place as this patch. So when the flag is set we know we can
> safely interpret this part of the padding field as a prefix as all valid
> calls from older user programs should not have a key length greater than
> 80 bytes.
>
> Would this be better? Programs compiled with the new headers could break
> on older kernels if they don't check the version, I don't know if that's
> a concern.
>
> Or should we just add these two new fields at the end of tcp_md5sig and
> use them only if the value of optlen in the parse function called from
> setsockopt is large enough?

I believe this is the deferrable way to handle this.

But note that old kernels would not send an error back, if an
application tries the new semantic.

2017-06-10 02:14:54

by Ivan Delalande

[permalink] [raw]

Subject: [PATCH v2 1/2] tcp: md5: add an address prefix for key lookup

2017-06-10 02:14:53

by Ivan Delalande

[permalink] [raw]

Subject: [PATCH v2 2/2] tcp: md5: extend the tcp_md5sig struct to specify a key address prefix

Add a flag field and address prefix length at the end of the tcp_md5sig
structure so users can configure an address prefix length along with a
key. Make sure shorter option values are still accepted in
tcp_v4_parse_md5_keys and tcp_v6_parse_md5_keys to maintain backward
compatibility.

Signed-off-by: Bob Gilligan <[email protected]>
Signed-off-by: Eric Mowat <[email protected]>
Signed-off-by: Ivan Delalande <[email protected]>
---
include/uapi/linux/tcp.h | 8 ++++++++
net/ipv4/tcp_ipv4.c | 15 +++++++++++----
net/ipv6/tcp_ipv6.c | 24 +++++++++++++++++-------
3 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 38a2b07afdff..440a8d983e4b 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -233,6 +233,12 @@ enum {

/* for TCP_MD5SIG socket option */
#define TCP_MD5SIG_MAXKEYLEN 80
+/* original struct stopped at tcpm_key and must still be considered valid */
+#define TCP_MD5SIG_LEGACY_LEN (offsetof(struct tcp_md5sig, tcpm_key) + \
+ TCP_MD5SIG_MAXKEYLEN)
+
+/* tcp_md5sig flags */
+#define TCP_MD5SIG_FLAG_PREFIX 1 /* address prefix length */

struct tcp_md5sig {
struct __kernel_sockaddr_storage tcpm_addr; /* address associated */
@@ -240,6 +246,8 @@ struct tcp_md5sig {
__u16 tcpm_keylen; /* key length */
__u32 __tcpm_pad2; /* zero */
__u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN]; /* key (binary) */
+ __u8 tcpm_flags; /* flags */
+ __u8 tcpm_prefixlen; /* address prefix */
};

#endif /* _UAPI_LINUX_TCP_H */
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 51ca3bd5a8a3..96a56224b913 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1069,25 +1069,32 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
{
struct tcp_md5sig cmd;
struct sockaddr_in *sin = (struct sockaddr_in *)&cmd.tcpm_addr;
+ u8 prefixlen = 32;

- if (optlen < sizeof(cmd))
+ if (optlen < TCP_MD5SIG_LEGACY_LEN)
return -EINVAL;

- if (copy_from_user(&cmd, optval, sizeof(cmd)))
+ if (copy_from_user(&cmd, optval, min_t(size_t, sizeof(cmd), optlen)))
return -EFAULT;

if (sin->sin_family != AF_INET)
return -EINVAL;

+ if (optlen >= sizeof(cmd) && cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
+ prefixlen = cmd.tcpm_prefixlen;
+ if (prefixlen > 32)
+ return -EINVAL;
+ }
+
if (!cmd.tcpm_keylen)
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr,
- AF_INET, 32);
+ AF_INET, prefixlen);

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
return -EINVAL;

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr,
- AF_INET, 32, cmd.tcpm_key, cmd.tcpm_keylen,
+ AF_INET, prefixlen, cmd.tcpm_key, cmd.tcpm_keylen,
GFP_KERNEL);
}

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 5cf19dab60aa..aff909e19b3d 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -519,22 +519,32 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
{
struct tcp_md5sig cmd;
struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&cmd.tcpm_addr;
+ u8 prefixlen;

- if (optlen < sizeof(cmd))
+ if (optlen < TCP_MD5SIG_LEGACY_LEN)
return -EINVAL;

- if (copy_from_user(&cmd, optval, sizeof(cmd)))
+ if (copy_from_user(&cmd, optval, min_t(size_t, sizeof(cmd), optlen)))
return -EFAULT;

if (sin6->sin6_family != AF_INET6)
return -EINVAL;

+ if (optlen >= sizeof(cmd) && cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
+ prefixlen = cmd.tcpm_prefixlen;
+ if (prefixlen > 128 || (ipv6_addr_v4mapped(&sin6->sin6_addr) &&
+ prefixlen > 32))
+ return -EINVAL;
+ } else {
+ prefixlen = ipv6_addr_v4mapped(&sin6->sin6_addr) ? 32 : 128;
+ }
+
if (!cmd.tcpm_keylen) {
if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
- AF_INET, 32);
+ AF_INET, prefixlen);
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
- AF_INET6, 128);
+ AF_INET6, prefixlen);
}

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
@@ -542,12 +552,12 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,

if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
- AF_INET, 32, cmd.tcpm_key,
+ AF_INET, prefixlen, cmd.tcpm_key,
cmd.tcpm_keylen, GFP_KERNEL);

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
- AF_INET6, 128, cmd.tcpm_key, cmd.tcpm_keylen,
- GFP_KERNEL);
+ AF_INET6, prefixlen, cmd.tcpm_key,
+ cmd.tcpm_keylen, GFP_KERNEL);
}

static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp,
--
2.13.1

2017-06-10 22:58:14

by David Miller

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] tcp: md5: extend the tcp_md5sig struct to specify a key address prefix

From: Ivan Delalande <[email protected]>
Date: Fri, 9 Jun 2017 19:14:49 -0700

> Add a flag field and address prefix length at the end of the tcp_md5sig
> structure so users can configure an address prefix length along with a
> key. Make sure shorter option values are still accepted in
> tcp_v4_parse_md5_keys and tcp_v6_parse_md5_keys to maintain backward
> compatibility.
>
> Signed-off-by: Bob Gilligan <[email protected]>
> Signed-off-by: Eric Mowat <[email protected]>
> Signed-off-by: Ivan Delalande <[email protected]>

As I believe was previously stated, the problem with this approach is
that if a new tool requests the prefix length and is run on an older
kernel, the kernel will return success even though the prefix length
was not taken into account.

We do not want to get a success back when the operation requested was
not performed.

2017-06-12 01:20:53

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] tcp: md5: add an address prefix for key lookup

Hi Ivan,

[auto build test WARNING on net/master]
[also build test WARNING on v4.12-rc4 next-20170609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Ivan-Delalande/tcp-md5-add-an-address-prefix-for-key-lookup/20170611-184237
config: x86_64-randconfig-s2-06120830 (attached as .config)
compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

All warnings (new ones prefixed by >>):

net//ipv4/tcp_ipv4.c: In function 'tcp_md5_do_lookup':
>> net//ipv4/tcp_ipv4.c:908: warning: unused variable 'size'

vim +/size +908 net//ipv4/tcp_ipv4.c

^1da177e4 Linus Torvalds 2005-04-16 892 }
^1da177e4 Linus Torvalds 2005-04-16 893
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 894 #ifdef CONFIG_TCP_MD5SIG
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 895 /*
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 896 * RFC2385 MD5 checksumming requires a mapping of
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 897 * IP address->MD5 Key.
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 898 * We need to maintain these in the sk structure.
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 899 */
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 900
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 901 /* Find the Key structure for an address. */
b83e3deb9 Eric Dumazet 2015-09-25 902 struct tcp_md5sig_key *tcp_md5_do_lookup(const struct sock *sk,
a915da9b6 Eric Dumazet 2012-01-31 903 const union tcp_md5_addr *addr,
a915da9b6 Eric Dumazet 2012-01-31 904 int family)
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 905 {
fd3a154a0 Eric Dumazet 2015-03-24 906 const struct tcp_sock *tp = tcp_sk(sk);
a915da9b6 Eric Dumazet 2012-01-31 907 struct tcp_md5sig_key *key;
a915da9b6 Eric Dumazet 2012-01-31 @908 unsigned int size = sizeof(struct in_addr);
fd3a154a0 Eric Dumazet 2015-03-24 909 const struct tcp_md5sig_info *md5sig;
4b9e1d2ec Ivan Delalande 2017-06-09 910 __be32 mask;
4b9e1d2ec Ivan Delalande 2017-06-09 911 struct tcp_md5sig_key *best_match = NULL;
4b9e1d2ec Ivan Delalande 2017-06-09 912 bool match;
cfb6eeb4c YOSHIFUJI Hideaki 2006-11-14 913
a8afca032 Eric Dumazet 2012-01-31 914 /* caller either holds rcu_read_lock() or socket lock */
a8afca032 Eric Dumazet 2012-01-31 915 md5sig = rcu_dereference_check(tp->md5sig_info,
1e1d04e67 Hannes Frederic Sowa 2016-04-05 916 lockdep_sock_is_held(sk));

:::::: The code at line 908 was first introduced by commit
:::::: a915da9b69273815527ccb3789421cb7027b545b tcp: md5: rcu conversion

:::::: TO: Eric Dumazet <[email protected]>
:::::: CC: David S. Miller <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (2.94 kB)
.config.gz (29.27 kB)
Download all attachments

2017-06-12 22:49:17

by Ivan Delalande

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] tcp: md5: extend the tcp_md5sig struct to specify a key address prefix

On Sat, Jun 10, 2017 at 06:58:11PM -0400, David Miller wrote:
> From: Ivan Delalande <[email protected]>
> Date: Fri, 9 Jun 2017 19:14:49 -0700
>
> > Add a flag field and address prefix length at the end of the tcp_md5sig
> > structure so users can configure an address prefix length along with a
> > key. Make sure shorter option values are still accepted in
> > tcp_v4_parse_md5_keys and tcp_v6_parse_md5_keys to maintain backward
> > compatibility.
> >
> > Signed-off-by: Bob Gilligan <[email protected]>
> > Signed-off-by: Eric Mowat <[email protected]>
> > Signed-off-by: Ivan Delalande <[email protected]>
>
> As I believe was previously stated, the problem with this approach is
> that if a new tool requests the prefix length and is run on an older
> kernel, the kernel will return success even though the prefix length
> was not taken into account.
>
> We do not want to get a success back when the operation requested was
> not performed.

Ah yeah that's right, sorry, definitely not great.

So I guess our only other option is to add a new socket option, like
TCP_MD5SIG_EXT which would use the extended version of struct tcp_md5sig
from this patch. Is it justified for this feature, or do you see any
other way to achieve this?

Thanks,
--
Ivan Delalande
Arista Networks

2017-06-16 01:07:11

by Ivan Delalande

[permalink] [raw]

Subject: [PATCH v3 2/2] tcp: md5: add TCP_MD5SIG_EXT socket option to set a key address prefix

Replace first padding in the tcp_md5sig structure with a new flag field
and address prefix length so it can be specified when configuring a new
key for TCP MD5 signature. The tcpm_flags field will only be used if the
socket option is TCP_MD5SIG_EXT to avoid breaking existing programs, and
tcpm_prefixlen only when the TCP_MD5SIG_FLAG_PREFIX flag is set.

Signed-off-by: Bob Gilligan <[email protected]>
Signed-off-by: Eric Mowat <[email protected]>
Signed-off-by: Ivan Delalande <[email protected]>
---
include/net/tcp.h | 1 +
include/uapi/linux/tcp.h | 9 +++++++--
net/ipv4/tcp.c | 3 ++-
net/ipv4/tcp_ipv4.c | 16 ++++++++++++----
net/ipv6/tcp_ipv6.c | 25 ++++++++++++++++++-------
5 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 2b68023ab095..575f95cb8275 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1802,6 +1802,7 @@ struct tcp_sock_af_ops {
const struct sock *sk,
const struct sk_buff *skb);
int (*md5_parse)(struct sock *sk,
+ int optname,
char __user *optval,
int optlen);
#endif
diff --git a/include/uapi/linux/tcp.h b/include/uapi/linux/tcp.h
index 38a2b07afdff..9870b7f08f4f 100644
--- a/include/uapi/linux/tcp.h
+++ b/include/uapi/linux/tcp.h
@@ -117,6 +117,7 @@ enum {
#define TCP_SAVED_SYN 28 /* Get SYN headers recorded for connection */
#define TCP_REPAIR_WINDOW 29 /* Get/set window parameters */
#define TCP_FASTOPEN_CONNECT 30 /* Attempt FastOpen with connect */
+#define TCP_MD5SIG_EXT 31 /* TCP MD5 Signature with extensions */

struct tcp_repair_opt {
__u32 opt_code;
@@ -234,11 +235,15 @@ enum {
/* for TCP_MD5SIG socket option */
#define TCP_MD5SIG_MAXKEYLEN 80

+/* tcp_md5sig extension flags for TCP_MD5SIG_EXT */
+#define TCP_MD5SIG_FLAG_PREFIX 1 /* address prefix length */
+
struct tcp_md5sig {
struct __kernel_sockaddr_storage tcpm_addr; /* address associated */
- __u16 __tcpm_pad1; /* zero */
+ __u8 tcpm_flags; /* extension flags */
+ __u8 tcpm_prefixlen; /* address prefix */
__u16 tcpm_keylen; /* key length */
- __u32 __tcpm_pad2; /* zero */
+ __u32 __tcpm_pad; /* zero */
__u8 tcpm_key[TCP_MD5SIG_MAXKEYLEN]; /* key (binary) */
};

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 1e4c76d2b827..2a68221d2e55 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2666,8 +2666,9 @@ static int do_tcp_setsockopt(struct sock *sk, int level,

#ifdef CONFIG_TCP_MD5SIG
case TCP_MD5SIG:
+ case TCP_MD5SIG_EXT:
/* Read the IP->Key mappings from userspace */
- err = tp->af_specific->md5_parse(sk, optval, optlen);
+ err = tp->af_specific->md5_parse(sk, optname, optval, optlen);
break;
#endif
case TCP_USER_TIMEOUT:
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 51ca3bd5a8a3..81d6c16aecdc 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1064,11 +1064,12 @@ static void tcp_clear_md5_list(struct sock *sk)
}
}

-static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
- int optlen)
+static int tcp_v4_parse_md5_keys(struct sock *sk, int optname,
+ char __user *optval, int optlen)
{
struct tcp_md5sig cmd;
struct sockaddr_in *sin = (struct sockaddr_in *)&cmd.tcpm_addr;
+ u8 prefixlen = 32;

if (optlen < sizeof(cmd))
return -EINVAL;
@@ -1079,15 +1080,22 @@ static int tcp_v4_parse_md5_keys(struct sock *sk, char __user *optval,
if (sin->sin_family != AF_INET)
return -EINVAL;

+ if (optname == TCP_MD5SIG_EXT &&
+ cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
+ prefixlen = cmd.tcpm_prefixlen;
+ if (prefixlen > 32)
+ return -EINVAL;
+ }
+
if (!cmd.tcpm_keylen)
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr,
- AF_INET, 32);
+ AF_INET, prefixlen);

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
return -EINVAL;

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin->sin_addr.s_addr,
- AF_INET, 32, cmd.tcpm_key, cmd.tcpm_keylen,
+ AF_INET, prefixlen, cmd.tcpm_key, cmd.tcpm_keylen,
GFP_KERNEL);
}

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 5cf19dab60aa..ae36442786ec 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -514,11 +514,12 @@ static struct tcp_md5sig_key *tcp_v6_md5_lookup(const struct sock *sk,
return tcp_v6_md5_do_lookup(sk, &addr_sk->sk_v6_daddr);
}

-static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
- int optlen)
+static int tcp_v6_parse_md5_keys(struct sock *sk, int optname,
+ char __user *optval, int optlen)
{
struct tcp_md5sig cmd;
struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)&cmd.tcpm_addr;
+ u8 prefixlen;

if (optlen < sizeof(cmd))
return -EINVAL;
@@ -529,12 +530,22 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,
if (sin6->sin6_family != AF_INET6)
return -EINVAL;

+ if (optname == TCP_MD5SIG_EXT &&
+ cmd.tcpm_flags & TCP_MD5SIG_FLAG_PREFIX) {
+ prefixlen = cmd.tcpm_prefixlen;
+ if (prefixlen > 128 || (ipv6_addr_v4mapped(&sin6->sin6_addr) &&
+ prefixlen > 32))
+ return -EINVAL;
+ } else {
+ prefixlen = ipv6_addr_v4mapped(&sin6->sin6_addr) ? 32 : 128;
+ }
+
if (!cmd.tcpm_keylen) {
if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
- AF_INET, 32);
+ AF_INET, prefixlen);
return tcp_md5_do_del(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
- AF_INET6, 128);
+ AF_INET6, prefixlen);
}

if (cmd.tcpm_keylen > TCP_MD5SIG_MAXKEYLEN)
@@ -542,12 +553,12 @@ static int tcp_v6_parse_md5_keys(struct sock *sk, char __user *optval,

if (ipv6_addr_v4mapped(&sin6->sin6_addr))
return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr.s6_addr32[3],
- AF_INET, 32, cmd.tcpm_key,
+ AF_INET, prefixlen, cmd.tcpm_key,
cmd.tcpm_keylen, GFP_KERNEL);

return tcp_md5_do_add(sk, (union tcp_md5_addr *)&sin6->sin6_addr,
- AF_INET6, 128, cmd.tcpm_key, cmd.tcpm_keylen,
- GFP_KERNEL);
+ AF_INET6, prefixlen, cmd.tcpm_key,
+ cmd.tcpm_keylen, GFP_KERNEL);
}

static int tcp_v6_md5_hash_headers(struct tcp_md5sig_pool *hp,
--
2.13.1

2017-06-16 01:07:10

by Ivan Delalande

[permalink] [raw]

Subject: [PATCH v3 1/2] tcp: md5: add an address prefix for key lookup

2017-06-19 17:51:54

by David Miller

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] tcp: md5: add an address prefix for key lookup

From: Ivan Delalande <[email protected]>
Date: Thu, 15 Jun 2017 18:07:06 -0700

> This allows the keys used for TCP MD5 signature to be used for whole
> range of addresses, specified with a prefix length, instead of only one
> address as it currently is.
>
> Signed-off-by: Bob Gilligan <[email protected]>
> Signed-off-by: Eric Mowat <[email protected]>
> Signed-off-by: Ivan Delalande <[email protected]>

Applied.

2017-06-19 17:52:26

by David Miller

[permalink] [raw]

Subject: Re: [PATCH v3 2/2] tcp: md5: add TCP_MD5SIG_EXT socket option to set a key address prefix

From: Ivan Delalande <[email protected]>
Date: Thu, 15 Jun 2017 18:07:07 -0700

> Replace first padding in the tcp_md5sig structure with a new flag field
> and address prefix length so it can be specified when configuring a new
> key for TCP MD5 signature. The tcpm_flags field will only be used if the
> socket option is TCP_MD5SIG_EXT to avoid breaking existing programs, and
> tcpm_prefixlen only when the TCP_MD5SIG_FLAG_PREFIX flag is set.
>
> Signed-off-by: Bob Gilligan <[email protected]>
> Signed-off-by: Eric Mowat <[email protected]>
> Signed-off-by: Ivan Delalande <[email protected]>

Applied but I had to renumber TCP_MD5SIG_EXT to 32 since 31 is already
taken by TCP_ULP in my tree.

It's a shame we had to add a new sockopt number to do this, but I can't
think of a better idea.

Thanks.