Re: [PATCH v3 04/19] tcp: add datapath logic for PSP with inline key exchange

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Daniel Zahka wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> 
> Add validation points and state propagation to support PSP key
> exchange inline, on TCP connections. The expectation is that
> application will use some well established mechanism like TLS
> handshake to establish a secure channel over the connection and
> if both endpoints are PSP-capable - exchange and install PSP keys.
> Because the connection can existing in PSP-unsecured and PSP-secured
> state we need to make sure that there are no race conditions or
> retransmission leaks.
> 
> On Tx - mark packets with the skb->decrypted bit when PSP key
> is at the enqueue time. Drivers should only encrypt packets with
> this bit set. This prevents retransmissions getting encrypted when
> original transmission was not. Similarly to TLS, we'll use
> sk->sk_validate_xmit_skb to make sure PSP skbs can't "escape"
> via a PSP-unaware device without being encrypted.
> 
> On Rx - validation is done under socket lock. This moves the validation
> point later than xfrm, for example. Please see the documentation patch
> for more details on the flow of securing a connection, but for
> the purpose of this patch what's important is that we want to
> enforce the invariant that once connection is secured any skb
> in the receive queue has been encrypted with PSP.
> 
> Add trivialities like GRO and coalescing checks.
> 
> This change only adds the validation points, for ease of review.
> Subsequent change will add the ability to install keys, and flesh
> the enforcement logic out
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> Co-developed-by: Daniel Zahka <daniel.zahka@gmail.com>
> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>

>  /**
>   *	inet_twsk_bind_unhash - unhash a timewait socket from bind hash
> @@ -218,6 +218,7 @@ struct inet_timewait_sock *inet_twsk_alloc(const struct sock *sk,
>  		refcount_set(&tw->tw_refcnt, 0);
>  
>  		__module_get(tw->tw_prot->owner);
> +		psp_twsk_init(tw, (struct sock *)sk);

Is it possible to avoid the need for a cast here? Can psp_sk_assoc
take a const pointer?

> @@ -689,6 +690,7 @@ void tcp_skb_entail(struct sock *sk, struct sk_buff *skb)
>  	tcb->seq     = tcb->end_seq = tp->write_seq;
>  	tcb->tcp_flags = TCPHDR_ACK;
>  	__skb_header_release(skb);
> +	psp_enqueue_set_decrypted(sk, skb);

If touching the tcp hot path, maybe a static branch.

> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index d0f49e6e3e35..79337028f3a5 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -104,9 +104,12 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
>  	struct tcp_timewait_sock *tcptw = tcp_twsk((struct sock *)tw);
>  	u32 rcv_nxt = READ_ONCE(tcptw->tw_rcv_nxt);
>  	struct tcp_options_received tmp_opt;
> +	enum skb_drop_reason psp_drop;
>  	bool paws_reject = false;
>  	int ts_recent_stamp;
>  
> +	psp_drop = psp_twsk_rx_policy_check(tw, skb);
> +

Why not return immediately here if the policy check fails, similar to
the non-timewait path?

>  	tmp_opt.saw_tstamp = 0;
>  	ts_recent_stamp = READ_ONCE(tcptw->tw_ts_recent_stamp);
>  	if (th->doff > (sizeof(*th) >> 2) && ts_recent_stamp) {
> @@ -124,6 +127,9 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
>  	if (READ_ONCE(tw->tw_substate) == TCP_FIN_WAIT2) {
>  		/* Just repeat all the checks of tcp_rcv_state_process() */
>  
> +		if (psp_drop)
> +			goto out_put;
> +
>  		/* Out of window, send ACK */
>  		if (paws_reject ||
>  		    !tcp_in_window(TCP_SKB_CB(skb)->seq, TCP_SKB_CB(skb)->end_seq,
> @@ -194,6 +200,9 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
>  	     (TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq || th->rst))) {
>  		/* In window segment, it may be only reset or bare ack. */
>  
> +		if (psp_drop)
> +			goto out_put;
> +
>  		if (th->rst) {
>  			/* This is TIME_WAIT assassination, in two flavors.
>  			 * Oh well... nobody has a sufficient solution to this
> @@ -247,6 +256,9 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
>  		return TCP_TW_SYN;
>  	}
>  
> +	if (psp_drop)
> +		goto out_put;
> +
>  	if (paws_reject) {
>  		*drop_reason = SKB_DROP_REASON_TCP_RFC7323_TW_PAWS;
>  		__NET_INC_STATS(twsk_net(tw), LINUX_MIB_PAWS_TW_REJECTED);
> @@ -265,6 +277,8 @@ tcp_timewait_state_process(struct inet_timewait_sock *tw, struct sk_buff *skb,
>  		return tcp_timewait_check_oow_rate_limit(
>  			tw, skb, LINUX_MIB_TCPACKSKIPPEDTIMEWAIT);
>  	}
> +
> +out_put:
>  	inet_twsk_put(tw);
>  	return TCP_TW_SUCCESS;
>  }