Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Leonard Crestez <cdleonard@gmail.com>
To: Dmitry Safonov <0x7f454c46@gmail.com>
Cc: Eric Dumazet <edumazet@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Kuniyuki Iwashima <kuniyu@amazon.co.jp>,
	David Ahern <dsahern@kernel.org>,
	Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>,
	Jakub Kicinski <kuba@kernel.org>,
	Yuchung Cheng <ycheng@google.com>,
	Francesco Ruggeri <fruggeri@arista.com>,
	Mat Martineau <mathew.j.martineau@linux.intel.com>,
	Christoph Paasch <cpaasch@apple.com>,
	Ivan Delalande <colona@arista.com>,
	Priyaranjan Jha <priyarjha@google.com>,
	Menglong Dong <dong.menglong@zte.com.cn>,
	open list <linux-kernel@vger.kernel.org>,
	linux-crypto@vger.kernel.org,
	Network Development <netdev@vger.kernel.org>,
	Dmitry Safonov <dima@arista.com>
Subject: Re: [RFCv2 1/9] tcp: authopt: Initial support and key management
Date: Wed, 11 Aug 2021 11:29:04 +0300	[thread overview]
Message-ID: <1e2848fb-1538-94aa-0431-636fa314a36d@gmail.com> (raw)
In-Reply-To: <CAJwJo6aicw_KGQSM5U1=0X11QfuNf2dMATErSymytmpf75W=tA@mail.gmail.com>

On 8/10/21 11:41 PM, Dmitry Safonov wrote:
> Hi Leonard,
> 
> On Tue, 10 Aug 2021 at 02:50, Leonard Crestez <cdleonard@gmail.com> wrote:
> [..]
>> +/* 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.

>> +       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.

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

> 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?

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

>> +       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.
> 
> [..]
>> +++ b/include/uapi/linux/tcp.h
>> @@ -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
>>
>>   #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)

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.

> [..]
>> +/**
>> + * 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),
>> +};
>> +
>> +/**
>> + * struct tcp_authopt - Per-socket options related to TCP Authentication Option
>> + */
>> +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)

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.

> [..]
>> +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 */
>> +       __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.

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.

> 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.

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.

> [..]
>> +#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;
>> +               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.

> [..]
>> +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)

Yes, this seems better. I think there are several socket locking 
mistakes in later commits. The patch adding support for detailed keyid 
control probably needs bh_lock_sock to avoid races between key selection 
and caching on send and key manipulation by userspace.

>> +       /* 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))?

It's extended in a later commit to add detailed keyid control.

> [..]
>> +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)
> 
> [..]
>> +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.
> 
> [..]
>> +       /* 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.

OK. It only saves a tiny bit of memory on API misuse.

>> +       /* 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.

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.

> 2. I think RFC says you must not allow a user to change an existing key:
>> 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.

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

--
Regards,
Leonard

  reply	other threads:[~2021-08-11  8:29 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-09 21:35 [RFCv2 0/9] tcp: Initial support for RFC5925 auth option Leonard Crestez
2021-08-09 21:35 ` [RFCv2 1/9] tcp: authopt: Initial support and key management Leonard Crestez
2021-08-10 20:41   ` Dmitry Safonov
2021-08-11  8:29     ` Leonard Crestez [this message]
2021-08-11 13:42       ` David Ahern
2021-08-11 19:11         ` Leonard Crestez
2021-08-11 20:26           ` Dmitry Safonov
2021-08-11 20:26           ` David Ahern
2021-08-11 14:31       ` Dmitry Safonov
2021-08-11 17:15         ` David Ahern
2021-08-11 20:12           ` Dmitry Safonov
2021-08-11 20:23             ` David Ahern
2021-08-11 19:08         ` Leonard Crestez
2021-08-12 19:46       ` Leonard Crestez
2021-08-09 21:35 ` [RFCv2 2/9] docs: Add user documentation for tcp_authopt Leonard Crestez
2021-08-09 21:35 ` [RFCv2 3/9] tcp: authopt: Add crypto initialization Leonard Crestez
2021-08-09 21:35 ` [RFCv2 4/9] tcp: authopt: Compute packet signatures Leonard Crestez
2021-08-09 21:35 ` [RFCv2 5/9] tcp: authopt: Hook into tcp core Leonard Crestez
2021-08-09 21:35 ` [RFCv2 6/9] tcp: authopt: Add key selection controls Leonard Crestez
2021-08-09 21:35 ` [RFCv2 7/9] tcp: authopt: Add snmp counters Leonard Crestez
2021-08-09 21:35 ` [RFCv2 8/9] selftests: Initial TCP-AO support for nettest Leonard Crestez
2021-08-09 21:35 ` [RFCv2 9/9] selftests: Initial TCP-AO support for fcnal-test Leonard Crestez
2021-08-11 13:46   ` David Ahern
2021-08-11 19:09     ` Leonard Crestez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1e2848fb-1538-94aa-0431-636fa314a36d@gmail.com \
    --to=cdleonard@gmail.com \
    --cc=0x7f454c46@gmail.com \
    --cc=colona@arista.com \
    --cc=cpaasch@apple.com \
    --cc=davem@davemloft.net \
    --cc=dima@arista.com \
    --cc=dong.menglong@zte.com.cn \
    --cc=dsahern@kernel.org \
    --cc=edumazet@google.com \
    --cc=fruggeri@arista.com \
    --cc=herbert@gondor.apana.org.au \
    --cc=kuba@kernel.org \
    --cc=kuniyu@amazon.co.jp \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathew.j.martineau@linux.intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=priyarjha@google.com \
    --cc=ycheng@google.com \
    --cc=yoshfuji@linux-ipv6.org \
    --subject='Re: [RFCv2 1/9] tcp: authopt: Initial support and key management' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).