Commit a4390e96 authored by Jakub Kicinski's avatar Jakub Kicinski
Browse files

Merge branch 'mptcp-reject-invalid-mp_join-requests-right-away' 

Florian Westphal says:

====================
mptcp: reject invalid mp_join requests right away

At the moment MPTCP can detect an invalid join request (invalid token,
max number of subflows reached, and so on) right away but cannot reject
the connection until the 3WHS has completed.
Instead the connection will complete and the subflow is reset afterwards.

To send the reset most information is already available, but we don't have
good spot where the reset could be sent:

1. The ->init_req callback is too early and also doesn't allow to return an
   error that could be used to inform the TCP stack that the SYN should be
   dropped.

2. The ->route_req callback lacks the skb needed to send a reset.

3. The ->send_synack callback is the best fit from the available hooks,
   but its called after the request socket has been inserted into the queue
   already. This means we'd have to remove it again right away.

From a technical point of view, the second hook would be best:
 1. Its before insertion into listener queue.
 2. If it returns NULL TCP will drop the packet for us.

Problem is that we'd have to pass the skb to the function just for MPTCP.

Paolo suggested to merge init_req and route_req callbacks instead:
This makes all info available to MPTCP -- a return value of NULL drops the
packet and MPTCP can send the reset if needed.

Because 'route_req' has a 'const struct sock *', this means either removal
of const qualifier, or a bit of code churn to pass 'const' in security land.

This does the latter; I did not find any spots that need write access to struct
sock.

To recap, the two alternatives are:
1. Solve it entirely in MPTCP: use the ->send_synack callback to
   unlink the request socket from the listener & drop it.
2. Avoid 'security' churn by removing the const qualifier.
====================

Link: https://lore.kernel.org/r/20201130153631.21872-1-fw@strlen.de


Signed-off-by: default avatarJakub Kicinski <kuba@kernel.org>
parents d4bff72c 3ecfbe3e

include/linux/lsm_audit.h

+1 −1
Original line number Diff line number Diff line
@@ -26,7 +26,7 @@ 


struct lsm_network_audit {

	int netif;

	struct sock *sk;

	const struct sock *sk;

	u16 family;

	__be16 dport;

	__be16 sport;

include/linux/lsm_hook_defs.h

+1 −1
Original line number Diff line number Diff line
@@ -301,7 +301,7 @@ LSM_HOOK(void, LSM_RET_VOID, sk_clone_security, const struct sock *sk, 
	 struct sock *newsk)

LSM_HOOK(void, LSM_RET_VOID, sk_getsecid, struct sock *sk, u32 *secid)

LSM_HOOK(void, LSM_RET_VOID, sock_graft, struct sock *sk, struct socket *parent)

LSM_HOOK(int, 0, inet_conn_request, struct sock *sk, struct sk_buff *skb,

LSM_HOOK(int, 0, inet_conn_request, const struct sock *sk, struct sk_buff *skb,

	 struct request_sock *req)

LSM_HOOK(void, LSM_RET_VOID, inet_csk_clone, struct sock *newsk,

	 const struct request_sock *req)

include/linux/security.h

+2 −2
Original line number Diff line number Diff line
@@ -1358,7 +1358,7 @@ void security_sk_clone(const struct sock *sk, struct sock *newsk); 
void security_sk_classify_flow(struct sock *sk, struct flowi *fl);

void security_req_classify_flow(const struct request_sock *req, struct flowi *fl);

void security_sock_graft(struct sock*sk, struct socket *parent);

int security_inet_conn_request(struct sock *sk,

int security_inet_conn_request(const struct sock *sk,

			struct sk_buff *skb, struct request_sock *req);

void security_inet_csk_clone(struct sock *newsk,

			const struct request_sock *req);
@@ -1519,7 +1519,7 @@ static inline void security_sock_graft(struct sock *sk, struct socket *parent) 
{

}



static inline int security_inet_conn_request(struct sock *sk,

static inline int security_inet_conn_request(const struct sock *sk,

			struct sk_buff *skb, struct request_sock *req)

{

	return 0;

include/net/tcp.h

+4 −5
Original line number Diff line number Diff line
@@ -2007,15 +2007,14 @@ struct tcp_request_sock_ops { 
					  const struct sock *sk,

					  const struct sk_buff *skb);

#endif

	void (*init_req)(struct request_sock *req,

			 const struct sock *sk_listener,

			 struct sk_buff *skb);

#ifdef CONFIG_SYN_COOKIES

	__u32 (*cookie_init_seq)(const struct sk_buff *skb,

				 __u16 *mss);

#endif

	struct dst_entry *(*route_req)(const struct sock *sk, struct flowi *fl,

				       const struct request_sock *req);

	struct dst_entry *(*route_req)(const struct sock *sk,

				       struct sk_buff *skb,

				       struct flowi *fl,

				       struct request_sock *req);

	u32 (*init_seq)(const struct sk_buff *skb);

	u32 (*init_ts_off)(const struct net *net, const struct sk_buff *skb);

	int (*send_synack)(const struct sock *sk, struct dst_entry *dst,

net/ipv4/tcp_input.c

+2 −7
Original line number Diff line number Diff line
@@ -6799,18 +6799,13 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops, 
	/* Note: tcp_v6_init_req() might override ir_iif for link locals */

	inet_rsk(req)->ir_iif = inet_request_bound_dev_if(sk, skb);



	af_ops->init_req(req, sk, skb);



	if (security_inet_conn_request(sk, skb, req))

	dst = af_ops->route_req(sk, skb, &fl, req);

	if (!dst)

		goto drop_and_free;



	if (tmp_opt.tstamp_ok)

		tcp_rsk(req)->ts_off = af_ops->init_ts_off(net, skb);



	dst = af_ops->route_req(sk, &fl, req);

	if (!dst)

		goto drop_and_free;



	if (!want_cookie && !isn) {

		/* Kill the following clause, if you dislike this way. */

		if (!net->ipv4.sysctl_tcp_syncookies &&

CRA Git | Maintained and supported by SUSTech CRA and CCSE