tcp: Initial support for RFC5925 auth option

include/uapi/linux/tcp.h +10 -0

+/**
+ * enum tcp_authopt_flag - flags for `tcp_authopt.flags`
+ */
+enum tcp_authopt_flag {
+       /**
+        * @TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED:
+        *      Configure behavior of segments with TCP-AO coming from hosts for which no
+        *      key is configured. The default recommended by RFC is to silently accept
+        *      such connections.
+        */

include/uapi/linux/tcp.h +12 -0

+struct tcp_authopt_key {
+       /** @flags: Combination of &enum tcp_authopt_key_flag */
+       __u32   flags;
+       /** @local_id: Local identifier */
+       __u32   local_id;
+       /** @send_id: keyid value for send */
+       __u8    send_id;
+       /** @recv_id: keyid value for receive */
+       __u8    recv_id;
+       /** @alg: One of &enum tcp_authopt_alg */
+       __u8    alg;
+       /** @keylen: Length of the key buffer */

net/ipv4/tcp.c +8 -0

+#ifdef CONFIG_TCP_AUTHOPT
+       case TCP_AUTHOPT: {
+               struct tcp_authopt info;
+
+               if (get_user(len, optlen))
+                       return -EFAULT;
+
+               lock_sock(sk);

MKT parameters are not changed. Instead, new MKTs can be installed, and a connection can change which MKT it uses.

Hi Leonard,

On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <[email protected]> wrote:
[..]

include/net/tcp_authopt.h line 11

Quoted 5 lines

include/net/tcp_authopt.h new file +5 -0

+/* Representation of a Master Key Tuple as per RFC5925 */
+struct tcp_authopt_key_info {
+       struct hlist_node node;
+       /* Local identifier */
+       u32 local_id;

There is no local_id in RFC5925, what's that?
An MKT is identified by (send_id, recv_id), together with (src_addr/src_port, dst_addr/dst_port).
Why introducing something new to already complicated RFC?

include/net/tcp_authopt.h line 18

Quoted 7 lines

include/net/tcp_authopt.h new file +7 -0

+       u32 flags;
+       /* Wire identifiers */
+       u8 send_id, recv_id;
+       u8 alg_id;
+       u8 keylen;
+       u8 key[TCP_AUTHOPT_MAXKEYLEN];
+       struct rcu_head rcu;

This is unaligned and will add padding.

I wonder if it's also worth saving some bytes by something like struct tcp_ao_key *key;

With

struct tcp_ao_key {
       u8 keylen;
       u8 key[0];
};

Hmm?

include/net/tcp_authopt.h line 29

Quoted 11 lines

include/net/tcp_authopt.h new file +11 -0

+       struct sockaddr_storage addr;
+};
+
+/* Per-socket information regarding tcp_authopt */
+struct tcp_authopt_info {
+       /* List of tcp_authopt_key_info */
+       struct hlist_head head;
+       u32 flags;
+       u32 src_isn;
+       u32 dst_isn;
+       struct rcu_head rcu;

Ditto, adds padding on 64-bit.

[..]

Quoted 6 lines

+++ b/include/uapi/linux/tcp.h

include/uapi/linux/tcp.h +0 -0

@@ -126,10 +126,12 @@ enum {
  #define TCP_INQ                        36      /* Notify bytes available to read as a cmsg on read */
 
  #define TCP_CM_INQ             TCP_INQ
 

include/uapi/linux/tcp.h enum { · line 132

Quoted 3 lines

include/uapi/linux/tcp.h +2 -0

  #define TCP_TX_DELAY           37      /* delay outgoing packets by XX usec */
+#define TCP_AUTHOPT                    38      /* TCP Authentication Option (RFC2385) */
+#define TCP_AUTHOPT_KEY                39      /* TCP Authentication Option update key (RFC2385) */

RFC2385 is md5 one.
Also, should there be TCP_AUTHOPT_ADD_KEY, TCP_AUTHOPT_DELTE_KEY instead of using flags inside setsocketopt()? (no hard feelings)

[..]

Quoted 14 lines

include/uapi/linux/tcp.h +14 -0

+/**
+ * enum tcp_authopt_flag - flags for `tcp_authopt.flags`
+ */
+enum tcp_authopt_flag {
+       /**
+        * @TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED:
+        *      Configure behavior of segments with TCP-AO coming from hosts for which no
+        *      key is configured. The default recommended by RFC is to silently accept
+        *      such connections.
+        */
+       TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED = (1 << 2),
+};
+
+/**

include/uapi/linux/tcp.h struct tcp_diag_md5sig { · line 366

Quoted 6 lines

+ * struct tcp_authopt - Per-socket options related to TCP Authentication Option
+ */

include/uapi/linux/tcp.h +4 -0

+struct tcp_authopt {
+       /** @flags: Combination of &enum tcp_authopt_flag */
+       __u32   flags;
+};

I'm not sure what's the use of enum here, probably: > #define TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED BIT(2)

[..]

Quoted 12 lines

include/uapi/linux/tcp.h +12 -0

+struct tcp_authopt_key {
+       /** @flags: Combination of &enum tcp_authopt_key_flag */
+       __u32   flags;
+       /** @local_id: Local identifier */
+       __u32   local_id;
+       /** @send_id: keyid value for send */
+       __u8    send_id;
+       /** @recv_id: keyid value for receive */
+       __u8    recv_id;
+       /** @alg: One of &enum tcp_authopt_alg */
+       __u8    alg;
+       /** @keylen: Length of the key buffer */

include/uapi/linux/tcp.h struct tcp_diag_md5sig { · line 415

Quoted 10 lines

include/uapi/linux/tcp.h +10 -0

+       __u8    keylen;
+       /** @key: Secret key */
+       __u8    key[TCP_AUTHOPT_MAXKEYLEN];
+       /**
+        * @addr: Key is only valid for this address
+        *
+        * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
+        */
+       struct __kernel_sockaddr_storage addr;
+};

It'll be an ABI if this is accepted. As it is - it can't support RFC5925 fully. Extending syscall ABI is painful. I think, even the initial ABI *must* support all possible features of the RFC.
In other words, there must be src_addr, src_port, dst_addr and dst_port. I can see from docs you've written you don't want to support a mix of different addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value but zero.
This will create an ABI that can be later extended to fully support RFC.

The same is about key: I don't think you need to define/use tcp_authopt_alg. Just use algo name - that way TCP-AO will automatically be able to use any algo supported by crypto engine.
See how xfrm does it, e.g.:
struct xfrm_algo_auth {
char alg_name[64];
unsigned int alg_key_len; /* in bits */
unsigned int alg_trunc_len; /* in bits */
char alg_key[0];
};

So you can let a user chose maclen instead of hard-coding it. Much more extendable than what you propose.

[..]

Quoted 8 lines

net/ipv4/tcp.c +8 -0

+#ifdef CONFIG_TCP_AUTHOPT
+       case TCP_AUTHOPT: {
+               struct tcp_authopt info;
+
+               if (get_user(len, optlen))
+                       return -EFAULT;
+
+               lock_sock(sk);

net/ipv4/tcp.c static int do_tcp_getsockopt(struct sock *sk, int level, · line 4248

Quoted 10 lines

net/ipv4/tcp.c +10 -0

+               tcp_get_authopt_val(sk, &info);
+               release_sock(sk);
+
+               len = min_t(unsigned int, len, sizeof(info));
+               if (put_user(len, optlen))
+                       return -EFAULT;
+               if (copy_to_user(optval, &info, len))
+                       return -EFAULT;
+               return 0;
+       }

I'm pretty sure it's not a good choice to write partly tcp_authopt. And a user has no way to check what's the correct len on this kernel. Instead of len = min_t(unsigned int, len, sizeof(info)), it should be if (len != sizeof(info))

     return -EINVAL;

[..]

net/ipv4/tcp_authopt.c line 47

Quoted 6 lines

net/ipv4/tcp_authopt.c new file +6 -0

+int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen)
+{
+       struct tcp_authopt opt;
+       struct tcp_authopt_info *info;
+
+       WARN_ON(!lockdep_sock_is_held(sk));

sock_owned_by_me(sk)

net/ipv4/tcp_authopt.c line 52

Quoted 4 lines

net/ipv4/tcp_authopt.c new file +4 -0

+       /* If userspace optlen is too short fill the rest with zeros */
+       if (optlen > sizeof(opt))
+               return -EINVAL;
+       memset(&opt, 0, sizeof(opt));

it's 4 bytes, why not just (optlen != sizeof(opt))?

[..]

net/ipv4/tcp_authopt.c line 70

Quoted 6 lines

net/ipv4/tcp_authopt.c new file +6 -0

+int tcp_get_authopt_val(struct sock *sk, struct tcp_authopt *opt)
+{
+       struct tcp_sock *tp = tcp_sk(sk);
+       struct tcp_authopt_info *info;
+
+       WARN_ON(!lockdep_sock_is_held(sk));

sock_owned_by_me(sk)

[..]

net/ipv4/tcp_authopt.c line 123

Quoted 12 lines

net/ipv4/tcp_authopt.c new file +12 -0

+int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen)
+{
+       struct tcp_authopt_key opt;
+       struct tcp_authopt_info *info;
+       struct tcp_authopt_key_info *key_info;
+
+       /* If userspace optlen is too short fill the rest with zeros */
+       if (optlen > sizeof(opt))
+               return -EINVAL;
+       memset(&opt, 0, sizeof(opt));
+       if (copy_from_sockptr(&opt, optval, optlen))
+               return -EFAULT;

Again, not a good practice to zero-extend struct. Enforce proper size with -EINVAL.

[..]

net/ipv4/tcp_authopt.c line 152

Quoted 10 lines

net/ipv4/tcp_authopt.c new file +10 -0

+       /* Initialize tcp_authopt_info if not already set */
+       info = __tcp_authopt_info_get_or_create(sk);
+       if (IS_ERR(info))
+               return PTR_ERR(info);
+
+       /* check key family */
+       if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
+               if (sk->sk_family != opt.addr.ss_family)
+                       return -EINVAL;
+       }

Probably, can be in the reverse order, so that
__tcp_authopt_info_get_or_create()
won't allocate memory.

net/ipv4/tcp_authopt.c line 160

Quoted 7 lines

net/ipv4/tcp_authopt.c new file +7 -0

+       /* If an old value exists for same local_id it is deleted */
+       key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
+       if (key_info)
+               tcp_authopt_key_del(sk, info, key_info);
+       key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
+       if (!key_info)
+               return -ENOMEM;

1. You don't need sock_kmalloc() together with tcp_authopt_key_del(). It just frees the memory and allocates it back straight away - no
sense in doing that.

2. I think RFC says you must not allow a user to change an existing key:

Quoted 2 lines

MKT parameters are not changed. Instead, new MKTs can be installed, and a connection can change which MKT it uses.

IIUC, it means that one can't just change an existing MKT, but one can remove and later
add MKT with the same (send_id, recv_id, src_addr/port, dst_addr/port).

So, a reasonable thing to do:
if (key_info)
return -EEXIST.

Quoted 5 lines · Dmitry Safonov

Hi Leonard,

On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <[email protected]> wrote:
[..]

Quoted 10 lines

include/net/tcp_authopt.h line 11

Quoted 5 lines

include/net/tcp_authopt.h new file +5 -0

+/* Representation of a Master Key Tuple as per RFC5925 */
+struct tcp_authopt_key_info {
+       struct hlist_node node;
+       /* Local identifier */
+       u32 local_id;

There is no local_id in RFC5925, what's that?
An MKT is identified by (send_id, recv_id), together with (src_addr/src_port, dst_addr/dst_port).
Why introducing something new to already complicated RFC?

It was there to simplify user interface and initial implementation.

But it seems that BGP listeners already needs to support multiple keychains for different peers so identifying the key by (send_id, recv_id, binding) is easier for userspace to work with. Otherwise they need to create their own local_id mapping internally.

Quoted 19 lines · Leonard Crestez

Quoted 5 lines · Dmitry Safonov

Hi Leonard,

On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <[email protected]> wrote:
[..]

Quoted 10 lines

include/net/tcp_authopt.h line 11

Quoted 5 lines

include/net/tcp_authopt.h new file +5 -0

+/* Representation of a Master Key Tuple as per RFC5925 */
+struct tcp_authopt_key_info {
+       struct hlist_node node;
+       /* Local identifier */
+       u32 local_id;

There is no local_id in RFC5925, what's that?
An MKT is identified by (send_id, recv_id), together with (src_addr/src_port, dst_addr/dst_port).
Why introducing something new to already complicated RFC?

It was there to simplify user interface and initial implementation.

Quoted 5 lines

But it seems that BGP listeners already needs to support multiple keychains for different peers so identifying the key by (send_id, recv_id, binding) is easier for userspace to work with. Otherwise they need to create their own local_id mapping internally.

any proposed simplification needs to be well explained and how it relates to the RFC spec.

On 11.08.2021 16:42, David Ahern wrote:

Quoted 3 lines

any proposed simplification needs to be well explained and how it relates to the RFC spec.

The local_id only exists between userspace and kernel so it's not really covered by the RFC.

There are objections to this and it seems to be unhelpful for userspace zo I will replace it with match by binding.

BTW: another somewhat dubious simplification is that I offloaded the RFC requirement to never add overlapping keys to userspace. So if userspace adds keys with same recvid that match the same TCP 4-tuple then connections will just start failing.

It's arguably fine to allow userspace misconfiguration to cause failures.

Quoted 26 lines · David Ahern

Quoted 24 lines · Leonard Crestez

Quoted 5 lines · Dmitry Safonov

Hi Leonard,

On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <[email protected]> wrote:
[..]

Quoted 10 lines

include/net/tcp_authopt.h line 11

Quoted 5 lines

include/net/tcp_authopt.h new file +5 -0

+/* Representation of a Master Key Tuple as per RFC5925 */
+struct tcp_authopt_key_info {
+       struct hlist_node node;
+       /* Local identifier */
+       u32 local_id;

There is no local_id in RFC5925, what's that?
An MKT is identified by (send_id, recv_id), together with (src_addr/src_port, dst_addr/dst_port).
Why introducing something new to already complicated RFC?

It was there to simplify user interface and initial implementation.

But it seems that BGP listeners already needs to support multiple keychains for different peers so identifying the key by (send_id, recv_id, binding) is easier for userspace to work with. Otherwise they need to create their own local_id mapping internally.

Quoted 2 lines

any proposed simplification needs to be well explained and how it relates to the RFC spec.

The local_id only exists between userspace and kernel so it's not really covered by the RFC.

There are objections to this and it seems to be unhelpful for userspace zo I will replace it with match by binding.

BTW: another somewhat dubious simplification is that I offloaded the RFC requirement to never add overlapping keys to userspace. So if userspace adds keys with same recvid that match the same TCP 4-tuple then connections will just start failing.

It's arguably fine to allow userspace misconfiguration to cause failures.

Signature · 2 lines

Regards,
Leonard

On 8/10/21 11:41 PM, Dmitry Safonov wrote:

Quoted 8 lines

Quoted 7 lines

+       u32 flags;
+       /* Wire identifiers */
+       u8 send_id, recv_id;
+       u8 alg_id;
+       u8 keylen;
+       u8 key[TCP_AUTHOPT_MAXKEYLEN];
+       struct rcu_head rcu;

Quoted 1 line

This is unaligned and will add padding.

Not clear padding matters. Moving rcu_head higher would avoid it, is that what you're suggesting.

Quoted 9 lines

I wonder if it's also worth saving some bytes by something like struct tcp_ao_key *key;

With
struct tcp_ao_key {
       u8 keylen;
       u8 key[0];
};

Quoted 1 line

Hmm?

This increases memory management complexity for very minor gains. Very few tcp_authopt_key will ever be created.

Quoted 8 lines

Quoted 4 lines

+#define TCP_AUTHOPT                    38      /* TCP Authentication Option (RFC2385) */
+#define TCP_AUTHOPT_KEY                39      /* TCP Authentication Option update key (RFC2385) */

RFC2385 is md5 one.
Also, should there be TCP_AUTHOPT_ADD_KEY, TCP_AUTHOPT_DELTE_KEY instead of using flags inside setsocketopt()? (no hard feelings)

Fixed RFC reference.

TCP_AUTHOPT_DELETE_KEY could be clearer, I just wanted to avoid bloating the sockopt space. But there's not any scarcity.

For reference tcp_md5 handles key deletion based on keylen == 0. This seems wrong to me, an empty key is in fact valid though not realistic.

If local_id is removed in favor of "full match on id and binding" then the delete sockopt would still need most of a full struct tcp_authopt_key anyway.

Quoted 1 line

TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED BIT(2)

This is an enum because it looks nice in kernel-doc. I couldn't find a way to attach docs to a macro and include it somewhere else.

BTW, the enum gains more members later.

As for BIT() it doesn't see to be allowed in uapi and there were recent changes removing such usage.

Quoted 28 lines

[..]

Quoted 12 lines

(quoted diff) +12 -0

+struct tcp_authopt_key {
+       /** @flags: Combination of &enum tcp_authopt_key_flag */
+       __u32   flags;
+       /** @local_id: Local identifier */
+       __u32   local_id;
+       /** @send_id: keyid value for send */
+       __u8    send_id;
+       /** @recv_id: keyid value for receive */
+       __u8    recv_id;
+       /** @alg: One of &enum tcp_authopt_alg */
+       __u8    alg;
+       /** @keylen: Length of the key buffer */

Review

Quoted 10 lines

(quoted diff) +10 -0

+       __u8    keylen;
+       /** @key: Secret key */
+       __u8    key[TCP_AUTHOPT_MAXKEYLEN];
+       /**
+        * @addr: Key is only valid for this address
+        *
+        * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
+        */
+       struct __kernel_sockaddr_storage addr;
+};

It'll be an ABI if this is accepted. As it is - it can't support RFC5925 fully.
Extending syscall ABI is painful. I think, even the initial ABI *must*

Quoted 8 lines

support
all possible features of the RFC.
In other words, there must be src_addr, src_port, dst_addr and dst_port. I can see from docs you've written you don't want to support a mix of different
addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value but zero.
This will create an ABI that can be later extended to fully support RFC.

RFC states that MKT connection identifiers can be specified using ranges and wildcards and the details are up to the implementation. Keys are *NOT* just bound to a classical TCP 4-tuple.

* src_addr and src_port is implicit in socket binding. Maybe in theory src_addr they could apply for a server socket bound to 0.0.0.0:PORT but userspace can just open more sockets.
* dst_port is not supported by MD5 and I can't think of any useful usecase. This is either well known (179 for BGP) or auto-allocated.
* tcp_md5 was recently enhanced allow a "prefixlen" for addr and "l3mdev" ifindex binding.

This last point shows that the binding features people require can't be easily predicted in advance so it's better to allow the rules to be extended.

Quoted 12 lines

The same is about key: I don't think you need to define/use tcp_authopt_alg.
Just use algo name - that way TCP-AO will automatically be able to use any algo supported by crypto engine.
See how xfrm does it, e.g.:
struct xfrm_algo_auth {
     char        alg_name[64];
     unsigned int    alg_key_len;    /* in bits */
     unsigned int    alg_trunc_len;  /* in bits */
     char        alg_key[0];
};

Quoted 2 lines

So you can let a user chose maclen instead of hard-coding it. Much more extendable than what you propose.

This complicates ABI and implementation with features that are not needed. I'd much rather only expose an enum of real-world tcp-ao algorithms.

Quoted 18 lines

[..]

Quoted 16 lines

(quoted diff) +16 -0

+#ifdef CONFIG_TCP_AUTHOPT
+       case TCP_AUTHOPT: {
+               struct tcp_authopt info;
+
+               if (get_user(len, optlen))
+                       return -EFAULT;
+
+               lock_sock(sk);
+               tcp_get_authopt_val(sk, &info);
+               release_sock(sk);
+
+               len = min_t(unsigned int, len, sizeof(info));
+               if (put_user(len, optlen))
+                       return -EFAULT;
+               if (copy_to_user(optval, &info, len))
+                       return -EFAULT;

Quoted 8 lines

Quoted 2 lines

+               return 0;
+       }

I'm pretty sure it's not a good choice to write partly tcp_authopt. And a user has no way to check what's the correct len on this kernel. Instead of len = min_t(unsigned int, len, sizeof(info)), it should be if (len != sizeof(info))
     return -EINVAL;

Purpose is to allow sockopts to grow as md5 has grown.

Quoted 1 line · Leonard Crestez

On 8/10/21 11:41 PM, Dmitry Safonov wrote:

[..]

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

+       u32 flags;
+       /* Wire identifiers */
+       u8 send_id, recv_id;
+       u8 alg_id;
+       u8 keylen;
+       u8 key[TCP_AUTHOPT_MAXKEYLEN];
+       struct rcu_head rcu;

Quoted 4 lines

Quoted 1 line

This is unaligned and will add padding.

Not clear padding matters. Moving rcu_head higher would avoid it, is that what you're suggesting.

Yes.

Quoted 9 lines

Quoted 9 lines

I wonder if it's also worth saving some bytes by something like struct tcp_ao_key *key;

With
struct tcp_ao_key {
       u8 keylen;
       u8 key[0];
};

Quoted 4 lines

Quoted 1 line

Hmm?

This increases memory management complexity for very minor gains. Very few tcp_authopt_key will ever be created.

The change doesn't seem to be big, like:

net/ipv4/tcp_authopt.c +0 -0

@@ -422,8 +422,16 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t

optval, unsig>

        key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
        if (key_info)
                tcp_authopt_key_del(sk, info, key_info);

net/ipv4/tcp_authopt.c +2 -0

+
+       key = sock_kmalloc(sk, sizeof(*key) + opt.keylen, GFP_KERNEL |

__GFP_ZERO);

net/ipv4/tcp_authopt.c +5 -0

+       if (!key) {
+               tcp_authopt_alg_release(alg);
+               return -ENOMEM;
+       }
+
        key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL |

__GFP_ZERO);
        if (!key_info) {
+               sock_kfree_s(sk, key, sizeof(*key) + opt.keylen);
                tcp_authopt_alg_release(alg);
                return -ENOMEM;
        }

I don't know, probably it'll be enough for every user to limit their keys by length of 80, but if one day it won't be enough - this ABI will be painful to extend.

[..]

Quoted 14 lines

Quoted 8 lines

Quoted 4 lines

+#define TCP_AUTHOPT                    38      /* TCP Authentication Option (RFC2385) */
+#define TCP_AUTHOPT_KEY                39      /* TCP Authentication Option update key (RFC2385) */

RFC2385 is md5 one.
Also, should there be TCP_AUTHOPT_ADD_KEY, TCP_AUTHOPT_DELTE_KEY instead of using flags inside setsocketopt()? (no hard feelings)

Fixed RFC reference.

TCP_AUTHOPT_DELETE_KEY could be clearer, I just wanted to avoid bloating the sockopt space. But there's not any scarcity.

Quoted 6 lines

For reference tcp_md5 handles key deletion based on keylen == 0. This seems wrong to me, an empty key is in fact valid though not realistic.

If local_id is removed in favor of "full match on id and binding" then the delete sockopt would still need most of a full struct tcp_authopt_key anyway.

Sounds like a plan.

[..]>> I'm not sure what's the use of enum here, probably: > #define

Quoted 4 lines

Quoted 1 line

TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED BIT(2)

This is an enum because it looks nice in kernel-doc. I couldn't find a way to attach docs to a macro and include it somewhere else.

Yeah, ok, seems like good justification.

Quoted 4 lines

BTW, the enum gains more members later.

As for BIT() it doesn't see to be allowed in uapi and there were recent changes removing such usage.

Ok, I just saw it's still used in include/uapi, but not aware of the removal.

Quoted 49 lines

Quoted 28 lines

[..]

Quoted 12 lines

(quoted diff) +12 -0

+struct tcp_authopt_key {
+       /** @flags: Combination of &enum tcp_authopt_key_flag */
+       __u32   flags;
+       /** @local_id: Local identifier */
+       __u32   local_id;
+       /** @send_id: keyid value for send */
+       __u8    send_id;
+       /** @recv_id: keyid value for receive */
+       __u8    recv_id;
+       /** @alg: One of &enum tcp_authopt_alg */
+       __u8    alg;
+       /** @keylen: Length of the key buffer */

Review

Quoted 10 lines

(quoted diff) +10 -0

+       __u8    keylen;
+       /** @key: Secret key */
+       __u8    key[TCP_AUTHOPT_MAXKEYLEN];
+       /**
+        * @addr: Key is only valid for this address
+        *
+        * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
+        */
+       struct __kernel_sockaddr_storage addr;
+};

It'll be an ABI if this is accepted. As it is - it can't support RFC5925 fully.
Extending syscall ABI is painful. I think, even the initial ABI *must*

Quoted 8 lines

support
all possible features of the RFC.
In other words, there must be src_addr, src_port, dst_addr and dst_port. I can see from docs you've written you don't want to support a mix of different
addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value but zero.
This will create an ABI that can be later extended to fully support RFC.

RFC states that MKT connection identifiers can be specified using ranges and wildcards and the details are up to the implementation. Keys are *NOT* just bound to a classical TCP 4-tuple.

* src_addr and src_port is implicit in socket binding. Maybe in theory src_addr they could apply for a server socket bound to 0.0.0.0:PORT but userspace can just open more sockets.
* dst_port is not supported by MD5 and I can't think of any useful usecase. This is either well known (179 for BGP) or auto-allocated.
* tcp_md5 was recently enhanced allow a "prefixlen" for addr and "l3mdev" ifindex binding.

Quoted 3 lines

This last point shows that the binding features people require can't be easily predicted in advance so it's better to allow the rules to be extended.

Yeah, I see both changes you mention went on easy way as they reused existing paddings in the ABI structures.
Ok, if you don't want to reserve src_addr/src_port/dst_addr/dst_port, than how about reserving some space for those instead?

Quoted 12 lines

Quoted 12 lines

The same is about key: I don't think you need to define/use tcp_authopt_alg.
Just use algo name - that way TCP-AO will automatically be able to use any algo supported by crypto engine.
See how xfrm does it, e.g.:
struct xfrm_algo_auth {
     char        alg_name[64];
     unsigned int    alg_key_len;    /* in bits */
     unsigned int    alg_trunc_len;  /* in bits */
     char        alg_key[0];
};

Quoted 6 lines

Quoted 2 lines

So you can let a user chose maclen instead of hard-coding it. Much more extendable than what you propose.

This complicates ABI and implementation with features that are not needed. I'd much rather only expose an enum of real-world tcp-ao algorithms.

I see it exactly the opposite way: a new enum unnecessary complicates ABI, instead of passing alg_name[] to crypto engine. No need to add any support in tcp-ao as the algorithms are already provided by kernel. That is how it transparently works for ipsec, why not for tcp-ao?

Quoted 27 lines

Quoted 26 lines

[..]

Quoted 8 lines

(quoted diff) +8 -0

+#ifdef CONFIG_TCP_AUTHOPT
+       case TCP_AUTHOPT: {
+               struct tcp_authopt info;
+
+               if (get_user(len, optlen))
+                       return -EFAULT;
+
+               lock_sock(sk);

Review

Quoted 10 lines

(quoted diff) +10 -0

+               tcp_get_authopt_val(sk, &info);
+               release_sock(sk);
+
+               len = min_t(unsigned int, len, sizeof(info));
+               if (put_user(len, optlen))
+                       return -EFAULT;
+               if (copy_to_user(optval, &info, len))
+                       return -EFAULT;
+               return 0;
+       }

I'm pretty sure it's not a good choice to write partly tcp_authopt. And a user has no way to check what's the correct len on this kernel. Instead of len = min_t(unsigned int, len, sizeof(info)), it should be if (len != sizeof(info))
     return -EINVAL;

Quoted 1 line

Purpose is to allow sockopts to grow as md5 has grown.

md5 has not grown. See above.

Another issue with your approach

net/ipv4/tcp_authopt.c +6 -0

+       /* If userspace optlen is too short fill the rest with zeros */
+       if (optlen > sizeof(opt))
+               return -EINVAL;
+       memset(&opt, 0, sizeof(opt));
+       if (copy_from_sockptr(&opt, optval, optlen))
+               return -EFAULT;

is that userspace compiled with updated/grew structure will fail on older kernel. So, no extension without breaking something is possible. Which also reminds me that currently you don't validate (opt.flags) for unknown by kernel flags.

Extending syscalls is impossible without breaking userspace if ABI is not designed with extensibility in mind. That was quite a big problem, and still is. Please, read this carefully:
https://lwn.net/Articles/830666/

That is why I'm suggesting you all those changes that will be harder to fix when/if your patches get accepted.
As an example how it should work see in copy_clone_args_from_user().

Quoted 2 lines · Dmitry Safonov

Quoted 1 line · Leonard Crestez

On 8/10/21 11:41 PM, Dmitry Safonov wrote:

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

I'm pretty sure it's not a good choice to write partly tcp_authopt. And a user has no way to check what's the correct len on this kernel. Instead of len = min_t(unsigned int, len, sizeof(info)), it should be if (len != sizeof(info))
     return -EINVAL;

Quoted 3 lines

Quoted 1 line

Purpose is to allow sockopts to grow as md5 has grown.

md5 has not grown. See above.

MD5 uapi has - e.g., 8917a777be3ba and 6b102db50cdde. We want similar capabilities for growth with this API.

Quoted 19 lines

Another issue with your approach

(quoted diff) +6 -0

+       /* If userspace optlen is too short fill the rest with zeros */
+       if (optlen > sizeof(opt))
+               return -EINVAL;
+       memset(&opt, 0, sizeof(opt));
+       if (copy_from_sockptr(&opt, optval, optlen))
+               return -EFAULT;

is that userspace compiled with updated/grew structure will fail on older kernel. So, no extension without breaking something is possible. Which also reminds me that currently you don't validate (opt.flags) for unknown by kernel flags.

Extending syscalls is impossible without breaking userspace if ABI is not designed with extensibility in mind. That was quite a big problem, and still is. Please, read this carefully:
https://lwn.net/Articles/830666/

Quoted 4 lines

That is why I'm suggesting you all those changes that will be harder to fix when/if your patches get accepted.
As an example how it should work see in copy_clone_args_from_user().

Look at how TCP_ZEROCOPY_RECEIVE has grown over releases as an example of how to properly handle this.

Hi David,

Quoted 3 lines · David Ahern

Quoted 2 lines · Dmitry Safonov

Quoted 1 line · Leonard Crestez

On 8/10/21 11:41 PM, Dmitry Safonov wrote:

[..]

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

I'm pretty sure it's not a good choice to write partly tcp_authopt. And a user has no way to check what's the correct len on this kernel. Instead of len = min_t(unsigned int, len, sizeof(info)), it should be if (len != sizeof(info))
     return -EINVAL;

Purpose is to allow sockopts to grow as md5 has grown.

Quoted 4 lines

Quoted 1 line

md5 has not grown. See above.

MD5 uapi has - e.g., 8917a777be3ba and 6b102db50cdde. We want similar capabilities for growth with this API.

So, you mean adding a new setsockopt when the struct has to be extended? Like TCP_AUTHOPT_EXT?

Quoted 3 lines

Look at how TCP_ZEROCOPY_RECEIVE has grown over releases as an example of how to properly handle this.

Exactly.

: switch (len) {
: case offsetofend(...)
: case offsetofend(...)

And than also:
: if (unlikely(len > sizeof(zc))) {
: err = check_zeroed_user(optval + sizeof(zc),
: len - sizeof(zc));

Does it sound similar to what I've written in my ABI review? And what the LWN article has in it.
Please, look again at the patch I replied to.

Thanks,
Dmitry

Quoted 10 lines · Leonard Crestez

Quoted 9 lines · Dmitry Safonov

I wonder if it's also worth saving some bytes by something like struct tcp_ao_key *key;

With
struct tcp_ao_key {
       u8 keylen;
       u8 key[0];
};

Quoted 4 lines

Quoted 1 line

Hmm?

This increases memory management complexity for very minor gains. Very few tcp_authopt_key will ever be created.

The change doesn't seem to be big, like:

net/ipv4/tcp_authopt.c +0 -0

@@ -422,8 +422,16 @@ int tcp_set_authopt_key(struct sock *sk, sockptr_t

optval, unsig>

         key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
         if (key_info)
                 tcp_authopt_key_del(sk, info, key_info);

net/ipv4/tcp_authopt.c +2 -0

+
+       key = sock_kmalloc(sk, sizeof(*key) + opt.keylen, GFP_KERNEL |

__GFP_ZERO);

net/ipv4/tcp_authopt.c +5 -0

+       if (!key) {
+               tcp_authopt_alg_release(alg);
+               return -ENOMEM;
+       }
+
         key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL |

__GFP_ZERO);
         if (!key_info) {
+               sock_kfree_s(sk, key, sizeof(*key) + opt.keylen);
                 tcp_authopt_alg_release(alg);
                 return -ENOMEM;
         }

I don't know, probably it'll be enough for every user to limit their keys by length of 80, but if one day it won't be enough - this ABI will be painful to extend.

Quoted 47 lines

Quoted 26 lines

Quoted 12 lines

(quoted diff) +12 -0

+struct tcp_authopt_key {
+       /** @flags: Combination of &enum tcp_authopt_key_flag */
+       __u32   flags;
+       /** @local_id: Local identifier */
+       __u32   local_id;
+       /** @send_id: keyid value for send */
+       __u8    send_id;
+       /** @recv_id: keyid value for receive */
+       __u8    recv_id;
+       /** @alg: One of &enum tcp_authopt_alg */
+       __u8    alg;
+       /** @keylen: Length of the key buffer */

Review

Quoted 10 lines

(quoted diff) +10 -0

+       __u8    keylen;
+       /** @key: Secret key */
+       __u8    key[TCP_AUTHOPT_MAXKEYLEN];
+       /**
+        * @addr: Key is only valid for this address
+        *
+        * Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set
+        */
+       struct __kernel_sockaddr_storage addr;
+};

It'll be an ABI if this is accepted. As it is - it can't support RFC5925 fully.
Extending syscall ABI is painful. I think, even the initial ABI *must*

Quoted 8 lines

support
all possible features of the RFC.
In other words, there must be src_addr, src_port, dst_addr and dst_port. I can see from docs you've written you don't want to support a mix of different
addr/port MKTs, so you can return -EINVAL or -ENOTSUPP for any value but zero.
This will create an ABI that can be later extended to fully support RFC.

RFC states that MKT connection identifiers can be specified using ranges and wildcards and the details are up to the implementation. Keys are *NOT* just bound to a classical TCP 4-tuple.

* src_addr and src_port is implicit in socket binding. Maybe in theory src_addr they could apply for a server socket bound to 0.0.0.0:PORT but userspace can just open more sockets.
* dst_port is not supported by MD5 and I can't think of any useful usecase. This is either well known (179 for BGP) or auto-allocated.
* tcp_md5 was recently enhanced allow a "prefixlen" for addr and "l3mdev" ifindex binding.

Quoted 3 lines

This last point shows that the binding features people require can't be easily predicted in advance so it's better to allow the rules to be extended.

Yeah, I see both changes you mention went on easy way as they reused existing paddings in the ABI structures.
Ok, if you don't want to reserve src_addr/src_port/dst_addr/dst_port, than how about reserving some space for those instead?

Quoted 15 lines

Quoted 14 lines

The same is about key: I don't think you need to define/use tcp_authopt_alg.
Just use algo name - that way TCP-AO will automatically be able to use any algo supported by crypto engine.
See how xfrm does it, e.g.:
struct xfrm_algo_auth {
     char        alg_name[64];
     unsigned int    alg_key_len;    /* in bits */ unsigned int    alg_trunc_len;  /* in bits */
     char        alg_key[0];
};

So you can let a user chose maclen instead of hard-coding it. Much more extendable than what you propose.

Quoted 3 lines

This complicates ABI and implementation with features that are not needed. I'd much rather only expose an enum of real-world tcp-ao algorithms.

I see it exactly the opposite way: a new enum unnecessary complicates ABI, instead of passing alg_name[] to crypto engine. No need to add any support in tcp-ao as the algorithms are already provided by kernel. That is how it transparently works for ipsec, why not for tcp-ao?

Quoted 26 lines

Quoted 17 lines

[..]

Quoted 16 lines

(quoted diff) +16 -0

+#ifdef CONFIG_TCP_AUTHOPT
+       case TCP_AUTHOPT: {
+               struct tcp_authopt info;
+
+               if (get_user(len, optlen))
+                       return -EFAULT;
+
+               lock_sock(sk);
+               tcp_get_authopt_val(sk, &info);
+               release_sock(sk);
+
+               len = min_t(unsigned int, len, sizeof(info));
+               if (put_user(len, optlen))
+                       return -EFAULT;
+               if (copy_to_user(optval, &info, len))
+                       return -EFAULT;

Quoted 8 lines

Quoted 2 lines

+               return 0;
+       }

I'm pretty sure it's not a good choice to write partly tcp_authopt. And a user has no way to check what's the correct len on this kernel. Instead of len = min_t(unsigned int, len, sizeof(info)), it should be if (len != sizeof(info))
     return -EINVAL;

Quoted 1 line

Purpose is to allow sockopts to grow as md5 has grown.

md5 has not grown. See above.

Another issue with your approach

net/ipv4/tcp_authopt.c +6 -0

+       /* If userspace optlen is too short fill the rest with zeros */
+       if (optlen > sizeof(opt))
+               return -EINVAL;
+       memset(&opt, 0, sizeof(opt));
+       if (copy_from_sockptr(&opt, optval, optlen))
+               return -EFAULT;

is that userspace compiled with updated/grew structure will fail on older kernel. So, no extension without breaking something is possible.

Which also reminds me that currently you don't validate (opt.flags) for unknown by kernel flags.

Extending syscalls is impossible without breaking userspace if ABI is not designed with extensibility in mind. That was quite a big problem, and still is. Please, read this carefully:
https://lwn.net/Articles/830666/

That is why I'm suggesting you all those changes that will be harder to fix when/if your patches get accepted.

Quoted 11 lines · Dmitry Safonov

On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <[email protected]>

Quoted 9 lines

+       /* If an old value exists for same local_id it is deleted */ +       key_info = __tcp_authopt_key_info_lookup(sk, info, opt.local_id);
+       if (key_info)
+               tcp_authopt_key_del(sk, info, key_info); +       key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
+       if (!key_info)
+               return -ENOMEM;

Quoted 3 lines

1. You don't need sock_kmalloc() together with tcp_authopt_key_del(). It just frees the memory and allocates it back straight away - no sense in doing that.

The list is scanned in multiple places in later commits using nothing but an rcu_read_lock, this means that keys can't be updated in-place.

Quoted 8 lines

2. I think RFC says you must not allow a user to change an existing key:

Quoted 2 lines

MKT parameters are not changed. Instead, new MKTs can be installed, and a connection can change which MKT it uses.

IIUC, it means that one can't just change an existing MKT, but one can remove and later
add MKT with the same (send_id, recv_id, src_addr/port, dst_addr/port).

Quoted 3 lines

So, a reasonable thing to do:
if (key_info)
     return -EEXIST.

You're right, making the user delete keys explicitly is better.

Just test that a correct password is required.

Quoted 2 lines · Leonard Crestez

Just test that a correct password is required.

This test suite needs to be comprehensive that the UAPI works as designed and fails when it should - cleanly and with an extack message as to why some config option fails. Tests should cover the datapath - that it works properly when it should and fails cleanly when it should not. If addresses are involved in the configuration, then the tests need to be written for non VRFs, with VRFs and default VRF since addresses are relative.

Also, in tree test suites are best for the maintenance of this code going forward.