[PATCH net-next 0/3] mptcp: reject invalid mp_join requests right away

[PATCH net-next 0/3] mptcp: reject invalid mp_join requests right away

[Florian Westphal]

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.

[PATCH net-next 1/3] security: add const qualifier to struct sock in various places

[Florian Westphal]

A followup change to tcp_request_sock_op would have to drop the 'const'
qualifier from the 'route_req' function as the
'security_inet_conn_request' call is moved there - and that function
expects a 'struct sock *'.

However, it turns out its also possible to add a const qualifier to
security_inet_conn_request instead.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 The code churn is unfortunate.  Alternative would be to change
 the function signature of ->route_req:
 struct dst_entry *(*route_req)(struct sock *sk, ...
 [ i.e., drop 'const' ].  Thoughts?

 include/linux/lsm_audit.h       | 2 +-
 include/linux/lsm_hook_defs.h   | 2 +-
 include/linux/security.h        | 4 ++--
 security/apparmor/include/net.h | 2 +-
 security/apparmor/lsm.c         | 2 +-
 security/apparmor/net.c         | 6 +++---
 security/lsm_audit.c            | 4 ++--
 security/security.c             | 2 +-
 security/selinux/hooks.c        | 2 +-
 security/smack/smack_lsm.c      | 4 ++--
 10 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
index 28f23b341c1c..cd23355d2271 100644
--- a/include/linux/lsm_audit.h
+++ b/include/linux/lsm_audit.h
@@ -26,7 +26,7 @@
 
 struct lsm_network_audit {
 	int netif;
-	struct sock *sk;
+	const struct sock *sk;
 	u16 family;
 	__be16 dport;
 	__be16 sport;
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 32a940117e7a..acc0494cceba 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -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)
diff --git a/include/linux/security.h b/include/linux/security.h
index bc2725491560..0df62735651b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -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;
diff --git a/security/apparmor/include/net.h b/security/apparmor/include/net.h
index 2431c011800d..aadb4b29fb66 100644
--- a/security/apparmor/include/net.h
+++ b/security/apparmor/include/net.h
@@ -107,6 +107,6 @@ int aa_sock_file_perm(struct aa_label *label, const char *op, u32 request,
 		      struct socket *sock);
 
 int apparmor_secmark_check(struct aa_label *label, char *op, u32 request,
-			   u32 secid, struct sock *sk);
+			   u32 secid, const struct sock *sk);
 
 #endif /* __AA_NET_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index ffeaee5ed968..1b0aba8eb723 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1147,7 +1147,7 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
 }
 
 #ifdef CONFIG_NETWORK_SECMARK
-static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
+static int apparmor_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
 				      struct request_sock *req)
 {
 	struct aa_sk_ctx *ctx = SK_CTX(sk);
diff --git a/security/apparmor/net.c b/security/apparmor/net.c
index fa0e85568450..e0c1b50d6edd 100644
--- a/security/apparmor/net.c
+++ b/security/apparmor/net.c
@@ -211,7 +211,7 @@ static int apparmor_secmark_init(struct aa_secmark *secmark)
 }
 
 static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
-			   struct common_audit_data *sa, struct sock *sk)
+			   struct common_audit_data *sa)
 {
 	int i, ret;
 	struct aa_perms perms = { };
@@ -244,13 +244,13 @@ static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
 }
 
 int apparmor_secmark_check(struct aa_label *label, char *op, u32 request,
-			   u32 secid, struct sock *sk)
+			   u32 secid, const struct sock *sk)
 {
 	struct aa_profile *profile;
 	DEFINE_AUDIT_SK(sa, op, sk);
 
 	return fn_for_each_confined(label, profile,
 				    aa_secmark_perm(profile, request, secid,
-						    &sa, sk));
+						    &sa));
 }
 #endif
diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 53d0d183db8f..078f9cdcd7f5 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -183,7 +183,7 @@ int ipv6_skb_to_auditdata(struct sk_buff *skb,
 
 
 static inline void print_ipv6_addr(struct audit_buffer *ab,
-				   struct in6_addr *addr, __be16 port,
+				   const struct in6_addr *addr, __be16 port,
 				   char *name1, char *name2)
 {
 	if (!ipv6_addr_any(addr))
@@ -322,7 +322,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 	}
 	case LSM_AUDIT_DATA_NET:
 		if (a->u.net->sk) {
-			struct sock *sk = a->u.net->sk;
+			const struct sock *sk = a->u.net->sk;
 			struct unix_sock *u;
 			struct unix_address *addr;
 			int len = 0;
diff --git a/security/security.c b/security/security.c
index a28045dc9e7f..6509f95d203f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2225,7 +2225,7 @@ void security_sock_graft(struct sock *sk, struct socket *parent)
 }
 EXPORT_SYMBOL(security_sock_graft);
 
-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)
 {
 	return call_int_hook(inet_conn_request, 0, sk, skb, req);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6b1826fc3658..6fa593006802 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5355,7 +5355,7 @@ static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
 	selinux_netlbl_sctp_sk_clone(sk, newsk);
 }
 
-static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
+static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
 				     struct request_sock *req)
 {
 	struct sk_security_struct *sksec = sk->sk_security;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 5c90b9fa4d40..3a62d6aa74a6 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3864,7 +3864,7 @@ static inline struct smack_known *smack_from_skb(struct sk_buff *skb)
  *
  * Returns smack_known of the IP options or NULL if that won't work.
  */
-static struct smack_known *smack_from_netlbl(struct sock *sk, u16 family,
+static struct smack_known *smack_from_netlbl(const struct sock *sk, u16 family,
 					     struct sk_buff *skb)
 {
 	struct netlbl_lsm_secattr secattr;
@@ -4114,7 +4114,7 @@ static void smack_sock_graft(struct sock *sk, struct socket *parent)
  * Returns 0 if a task with the packet label could write to
  * the socket, otherwise an error code
  */
-static int smack_inet_conn_request(struct sock *sk, struct sk_buff *skb,
+static int smack_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
 				   struct request_sock *req)
 {
 	u16 family = sk->sk_family;
-- 
2.26.2

Re: [PATCH net-next 1/3] security: add const qualifier to struct sock in various places

[Jakub Kicinski]

On Mon, 30 Nov 2020 16:36:29 +0100 Florian Westphal wrote:
> A followup change to tcp_request_sock_op would have to drop the 'const'
> qualifier from the 'route_req' function as the
> 'security_inet_conn_request' call is moved there - and that function
> expects a 'struct sock *'.
> 
> However, it turns out its also possible to add a const qualifier to
> security_inet_conn_request instead.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  The code churn is unfortunate.  Alternative would be to change
>  the function signature of ->route_req:
>  struct dst_entry *(*route_req)(struct sock *sk, ...
>  [ i.e., drop 'const' ].  Thoughts?

Security folks - is this okay to merge into net-next?

We can put it on a branch and pull into both trees if the risk 
of conflicts is high.

> diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
> index 28f23b341c1c..cd23355d2271 100644
> --- a/include/linux/lsm_audit.h
> +++ b/include/linux/lsm_audit.h
> @@ -26,7 +26,7 @@
>  
>  struct lsm_network_audit {
>  	int netif;
> -	struct sock *sk;
> +	const struct sock *sk;
>  	u16 family;
>  	__be16 dport;
>  	__be16 sport;
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 32a940117e7a..acc0494cceba 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -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)
> diff --git a/include/linux/security.h b/include/linux/security.h
> index bc2725491560..0df62735651b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -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;
> diff --git a/security/apparmor/include/net.h b/security/apparmor/include/net.h
> index 2431c011800d..aadb4b29fb66 100644
> --- a/security/apparmor/include/net.h
> +++ b/security/apparmor/include/net.h
> @@ -107,6 +107,6 @@ int aa_sock_file_perm(struct aa_label *label, const char *op, u32 request,
>  		      struct socket *sock);
>  
>  int apparmor_secmark_check(struct aa_label *label, char *op, u32 request,
> -			   u32 secid, struct sock *sk);
> +			   u32 secid, const struct sock *sk);
>  
>  #endif /* __AA_NET_H */
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index ffeaee5ed968..1b0aba8eb723 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1147,7 +1147,7 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
>  }
>  
>  #ifdef CONFIG_NETWORK_SECMARK
> -static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> +static int apparmor_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
>  				      struct request_sock *req)
>  {
>  	struct aa_sk_ctx *ctx = SK_CTX(sk);
> diff --git a/security/apparmor/net.c b/security/apparmor/net.c
> index fa0e85568450..e0c1b50d6edd 100644
> --- a/security/apparmor/net.c
> +++ b/security/apparmor/net.c
> @@ -211,7 +211,7 @@ static int apparmor_secmark_init(struct aa_secmark *secmark)
>  }
>  
>  static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
> -			   struct common_audit_data *sa, struct sock *sk)
> +			   struct common_audit_data *sa)
>  {
>  	int i, ret;
>  	struct aa_perms perms = { };
> @@ -244,13 +244,13 @@ static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
>  }
>  
>  int apparmor_secmark_check(struct aa_label *label, char *op, u32 request,
> -			   u32 secid, struct sock *sk)
> +			   u32 secid, const struct sock *sk)
>  {
>  	struct aa_profile *profile;
>  	DEFINE_AUDIT_SK(sa, op, sk);
>  
>  	return fn_for_each_confined(label, profile,
>  				    aa_secmark_perm(profile, request, secid,
> -						    &sa, sk));
> +						    &sa));
>  }
>  #endif
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 53d0d183db8f..078f9cdcd7f5 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -183,7 +183,7 @@ int ipv6_skb_to_auditdata(struct sk_buff *skb,
>  
>  
>  static inline void print_ipv6_addr(struct audit_buffer *ab,
> -				   struct in6_addr *addr, __be16 port,
> +				   const struct in6_addr *addr, __be16 port,
>  				   char *name1, char *name2)
>  {
>  	if (!ipv6_addr_any(addr))
> @@ -322,7 +322,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
>  	}
>  	case LSM_AUDIT_DATA_NET:
>  		if (a->u.net->sk) {
> -			struct sock *sk = a->u.net->sk;
> +			const struct sock *sk = a->u.net->sk;
>  			struct unix_sock *u;
>  			struct unix_address *addr;
>  			int len = 0;
> diff --git a/security/security.c b/security/security.c
> index a28045dc9e7f..6509f95d203f 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2225,7 +2225,7 @@ void security_sock_graft(struct sock *sk, struct socket *parent)
>  }
>  EXPORT_SYMBOL(security_sock_graft);
>  
> -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)
>  {
>  	return call_int_hook(inet_conn_request, 0, sk, skb, req);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 6b1826fc3658..6fa593006802 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5355,7 +5355,7 @@ static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
>  	selinux_netlbl_sctp_sk_clone(sk, newsk);
>  }
>  
> -static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> +static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
>  				     struct request_sock *req)
>  {
>  	struct sk_security_struct *sksec = sk->sk_security;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 5c90b9fa4d40..3a62d6aa74a6 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3864,7 +3864,7 @@ static inline struct smack_known *smack_from_skb(struct sk_buff *skb)
>   *
>   * Returns smack_known of the IP options or NULL if that won't work.
>   */
> -static struct smack_known *smack_from_netlbl(struct sock *sk, u16 family,
> +static struct smack_known *smack_from_netlbl(const struct sock *sk, u16 family,
>  					     struct sk_buff *skb)
>  {
>  	struct netlbl_lsm_secattr secattr;
> @@ -4114,7 +4114,7 @@ static void smack_sock_graft(struct sock *sk, struct socket *parent)
>   * Returns 0 if a task with the packet label could write to
>   * the socket, otherwise an error code
>   */
> -static int smack_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> +static int smack_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
>  				   struct request_sock *req)
>  {
>  	u16 family = sk->sk_family;

Re: [PATCH net-next 1/3] security: add const qualifier to struct sock in various places

[James Morris]

On Wed, 2 Dec 2020, Jakub Kicinski wrote:

> On Mon, 30 Nov 2020 16:36:29 +0100 Florian Westphal wrote:
> > A followup change to tcp_request_sock_op would have to drop the 'const'
> > qualifier from the 'route_req' function as the
> > 'security_inet_conn_request' call is moved there - and that function
> > expects a 'struct sock *'.
> > 
> > However, it turns out its also possible to add a const qualifier to
> > security_inet_conn_request instead.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  The code churn is unfortunate.  Alternative would be to change
> >  the function signature of ->route_req:
> >  struct dst_entry *(*route_req)(struct sock *sk, ...
> >  [ i.e., drop 'const' ].  Thoughts?
> 
> Security folks - is this okay to merge into net-next?
> 
> We can put it on a branch and pull into both trees if the risk 
> of conflicts is high.


Acked-by: James Morris <jamorris@linux.microsoft.com>

> 
> > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h
> > index 28f23b341c1c..cd23355d2271 100644
> > --- a/include/linux/lsm_audit.h
> > +++ b/include/linux/lsm_audit.h
> > @@ -26,7 +26,7 @@
> >  
> >  struct lsm_network_audit {
> >  	int netif;
> > -	struct sock *sk;
> > +	const struct sock *sk;
> >  	u16 family;
> >  	__be16 dport;
> >  	__be16 sport;
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 32a940117e7a..acc0494cceba 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -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)
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index bc2725491560..0df62735651b 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -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;
> > diff --git a/security/apparmor/include/net.h b/security/apparmor/include/net.h
> > index 2431c011800d..aadb4b29fb66 100644
> > --- a/security/apparmor/include/net.h
> > +++ b/security/apparmor/include/net.h
> > @@ -107,6 +107,6 @@ int aa_sock_file_perm(struct aa_label *label, const char *op, u32 request,
> >  		      struct socket *sock);
> >  
> >  int apparmor_secmark_check(struct aa_label *label, char *op, u32 request,
> > -			   u32 secid, struct sock *sk);
> > +			   u32 secid, const struct sock *sk);
> >  
> >  #endif /* __AA_NET_H */
> > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > index ffeaee5ed968..1b0aba8eb723 100644
> > --- a/security/apparmor/lsm.c
> > +++ b/security/apparmor/lsm.c
> > @@ -1147,7 +1147,7 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
> >  }
> >  
> >  #ifdef CONFIG_NETWORK_SECMARK
> > -static int apparmor_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> > +static int apparmor_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
> >  				      struct request_sock *req)
> >  {
> >  	struct aa_sk_ctx *ctx = SK_CTX(sk);
> > diff --git a/security/apparmor/net.c b/security/apparmor/net.c
> > index fa0e85568450..e0c1b50d6edd 100644
> > --- a/security/apparmor/net.c
> > +++ b/security/apparmor/net.c
> > @@ -211,7 +211,7 @@ static int apparmor_secmark_init(struct aa_secmark *secmark)
> >  }
> >  
> >  static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
> > -			   struct common_audit_data *sa, struct sock *sk)
> > +			   struct common_audit_data *sa)
> >  {
> >  	int i, ret;
> >  	struct aa_perms perms = { };
> > @@ -244,13 +244,13 @@ static int aa_secmark_perm(struct aa_profile *profile, u32 request, u32 secid,
> >  }
> >  
> >  int apparmor_secmark_check(struct aa_label *label, char *op, u32 request,
> > -			   u32 secid, struct sock *sk)
> > +			   u32 secid, const struct sock *sk)
> >  {
> >  	struct aa_profile *profile;
> >  	DEFINE_AUDIT_SK(sa, op, sk);
> >  
> >  	return fn_for_each_confined(label, profile,
> >  				    aa_secmark_perm(profile, request, secid,
> > -						    &sa, sk));
> > +						    &sa));
> >  }
> >  #endif
> > diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> > index 53d0d183db8f..078f9cdcd7f5 100644
> > --- a/security/lsm_audit.c
> > +++ b/security/lsm_audit.c
> > @@ -183,7 +183,7 @@ int ipv6_skb_to_auditdata(struct sk_buff *skb,
> >  
> >  
> >  static inline void print_ipv6_addr(struct audit_buffer *ab,
> > -				   struct in6_addr *addr, __be16 port,
> > +				   const struct in6_addr *addr, __be16 port,
> >  				   char *name1, char *name2)
> >  {
> >  	if (!ipv6_addr_any(addr))
> > @@ -322,7 +322,7 @@ static void dump_common_audit_data(struct audit_buffer *ab,
> >  	}
> >  	case LSM_AUDIT_DATA_NET:
> >  		if (a->u.net->sk) {
> > -			struct sock *sk = a->u.net->sk;
> > +			const struct sock *sk = a->u.net->sk;
> >  			struct unix_sock *u;
> >  			struct unix_address *addr;
> >  			int len = 0;
> > diff --git a/security/security.c b/security/security.c
> > index a28045dc9e7f..6509f95d203f 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -2225,7 +2225,7 @@ void security_sock_graft(struct sock *sk, struct socket *parent)
> >  }
> >  EXPORT_SYMBOL(security_sock_graft);
> >  
> > -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)
> >  {
> >  	return call_int_hook(inet_conn_request, 0, sk, skb, req);
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 6b1826fc3658..6fa593006802 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -5355,7 +5355,7 @@ static void selinux_sctp_sk_clone(struct sctp_endpoint *ep, struct sock *sk,
> >  	selinux_netlbl_sctp_sk_clone(sk, newsk);
> >  }
> >  
> > -static int selinux_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> > +static int selinux_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
> >  				     struct request_sock *req)
> >  {
> >  	struct sk_security_struct *sksec = sk->sk_security;
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index 5c90b9fa4d40..3a62d6aa74a6 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -3864,7 +3864,7 @@ static inline struct smack_known *smack_from_skb(struct sk_buff *skb)
> >   *
> >   * Returns smack_known of the IP options or NULL if that won't work.
> >   */
> > -static struct smack_known *smack_from_netlbl(struct sock *sk, u16 family,
> > +static struct smack_known *smack_from_netlbl(const struct sock *sk, u16 family,
> >  					     struct sk_buff *skb)
> >  {
> >  	struct netlbl_lsm_secattr secattr;
> > @@ -4114,7 +4114,7 @@ static void smack_sock_graft(struct sock *sk, struct socket *parent)
> >   * Returns 0 if a task with the packet label could write to
> >   * the socket, otherwise an error code
> >   */
> > -static int smack_inet_conn_request(struct sock *sk, struct sk_buff *skb,
> > +static int smack_inet_conn_request(const struct sock *sk, struct sk_buff *skb,
> >  				   struct request_sock *req)
> >  {
> >  	u16 family = sk->sk_family;
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH net-next 1/3] security: add const qualifier to struct sock in various places

[Jakub Kicinski]

On Fri, 4 Dec 2020 04:07:16 +1100 (AEDT) James Morris wrote:
> On Wed, 2 Dec 2020, Jakub Kicinski wrote:
> > On Mon, 30 Nov 2020 16:36:29 +0100 Florian Westphal wrote:  
> > > A followup change to tcp_request_sock_op would have to drop the 'const'
> > > qualifier from the 'route_req' function as the
> > > 'security_inet_conn_request' call is moved there - and that function
> > > expects a 'struct sock *'.
> > > 
> > > However, it turns out its also possible to add a const qualifier to
> > > security_inet_conn_request instead.
> > > 
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > ---
> > >  The code churn is unfortunate.  Alternative would be to change
> > >  the function signature of ->route_req:
> > >  struct dst_entry *(*route_req)(struct sock *sk, ...
> > >  [ i.e., drop 'const' ].  Thoughts?  
> > 
> > Security folks - is this okay to merge into net-next?
> > 
> > We can put it on a branch and pull into both trees if the risk 
> > of conflicts is high.  
> 
> Acked-by: James Morris <jamorris@linux.microsoft.com>

Thank you!

Into net-next it goes..

[PATCH net-next 2/3] tcp: merge 'init_req' and 'route_req' functions

[Florian Westphal]

The Multipath-TCP standard (RFC 8684) says that an MPTCP host should send
a TCP reset if the token in a MP_JOIN request is unknown.

At this time we don't do this, the 3whs completes and the 'new subflow'
is reset afterwards.  There are two ways to allow MPTCP to send the
reset.

1. override 'send_synack' callback and emit the rst from there.
   The drawback is that the request socket gets inserted into the
   listeners queue just to get removed again right away.

2. Send the reset from the 'route_req' function instead.
   This avoids the 'add&remove request socket', but route_req lacks the
   skb that is required to send the TCP reset.

Instead of just adding the skb to that function for MPTCP sake alone,
Paolo suggested to merge init_req and route_req functions.

This saves one indirection from syn processing path and provides the skb
to the merged function at the same time.

'send reset on unknown mptcp join token' is added in next patch.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Cc: Eric Dumazet <edumazet@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/net/tcp.h    |  9 ++++-----
 net/ipv4/tcp_input.c |  9 ++-------
 net/ipv4/tcp_ipv4.c  |  9 +++++++--
 net/ipv6/tcp_ipv6.c  |  9 +++++++--
 net/mptcp/subflow.c  | 36 ++++++++++++++++++++++++------------
 5 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index d5d22c411918..4525d6256321 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -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,
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index fb3a7750f623..9e8a6c1aa019 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -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 &&
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index e4b31e70bd30..af2338294598 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -1444,9 +1444,15 @@ static void tcp_v4_init_req(struct request_sock *req,
 }
 
 static struct dst_entry *tcp_v4_route_req(const struct sock *sk,
+					  struct sk_buff *skb,
 					  struct flowi *fl,
-					  const struct request_sock *req)
+					  struct request_sock *req)
 {
+	tcp_v4_init_req(req, sk, skb);
+
+	if (security_inet_conn_request(sk, skb, req))
+		return NULL;
+
 	return inet_csk_route_req(sk, &fl->u.ip4, req);
 }
 
@@ -1466,7 +1472,6 @@ const struct tcp_request_sock_ops tcp_request_sock_ipv4_ops = {
 	.req_md5_lookup	=	tcp_v4_md5_lookup,
 	.calc_md5_hash	=	tcp_v4_md5_hash_skb,
 #endif
-	.init_req	=	tcp_v4_init_req,
 #ifdef CONFIG_SYN_COOKIES
 	.cookie_init_seq =	cookie_v4_init_sequence,
 #endif
diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 992cbf3eb9e3..1a1510513739 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -828,9 +828,15 @@ static void tcp_v6_init_req(struct request_sock *req,
 }
 
 static struct dst_entry *tcp_v6_route_req(const struct sock *sk,
+					  struct sk_buff *skb,
 					  struct flowi *fl,
-					  const struct request_sock *req)
+					  struct request_sock *req)
 {
+	tcp_v6_init_req(req, sk, skb);
+
+	if (security_inet_conn_request(sk, skb, req))
+		return NULL;
+
 	return inet6_csk_route_req(sk, &fl->u.ip6, req, IPPROTO_TCP);
 }
 
@@ -851,7 +857,6 @@ const struct tcp_request_sock_ops tcp_request_sock_ipv6_ops = {
 	.req_md5_lookup	=	tcp_v6_md5_lookup,
 	.calc_md5_hash	=	tcp_v6_md5_hash_skb,
 #endif
-	.init_req	=	tcp_v6_init_req,
 #ifdef CONFIG_SYN_COOKIES
 	.cookie_init_seq =	cookie_v6_init_sequence,
 #endif
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 2e5c3f4da3a4..c55b8f176746 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -228,27 +228,39 @@ int mptcp_subflow_init_cookie_req(struct request_sock *req,
 }
 EXPORT_SYMBOL_GPL(mptcp_subflow_init_cookie_req);
 
-static void subflow_v4_init_req(struct request_sock *req,
-				const struct sock *sk_listener,
-				struct sk_buff *skb)
+static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
+					      struct sk_buff *skb,
+					      struct flowi *fl,
+					      struct request_sock *req)
 {
+	struct dst_entry *dst;
+
 	tcp_rsk(req)->is_mptcp = 1;
 
-	tcp_request_sock_ipv4_ops.init_req(req, sk_listener, skb);
+	dst = tcp_request_sock_ipv4_ops.route_req(sk, skb, fl, req);
+	if (!dst)
+		return NULL;
 
-	subflow_init_req(req, sk_listener, skb);
+	subflow_init_req(req, sk, skb);
+	return dst;
 }
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
-static void subflow_v6_init_req(struct request_sock *req,
-				const struct sock *sk_listener,
-				struct sk_buff *skb)
+static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
+					      struct sk_buff *skb,
+					      struct flowi *fl,
+					      struct request_sock *req)
 {
+	struct dst_entry *dst;
+
 	tcp_rsk(req)->is_mptcp = 1;
 
-	tcp_request_sock_ipv6_ops.init_req(req, sk_listener, skb);
+	dst = tcp_request_sock_ipv6_ops.route_req(sk, skb, fl, req);
+	if (!dst)
+		return NULL;
 
-	subflow_init_req(req, sk_listener, skb);
+	subflow_init_req(req, sk, skb);
+	return dst;
 }
 #endif
 
@@ -1398,7 +1410,7 @@ void __init mptcp_subflow_init(void)
 		panic("MPTCP: failed to init subflow request sock ops\n");
 
 	subflow_request_sock_ipv4_ops = tcp_request_sock_ipv4_ops;
-	subflow_request_sock_ipv4_ops.init_req = subflow_v4_init_req;
+	subflow_request_sock_ipv4_ops.route_req = subflow_v4_route_req;
 
 	subflow_specific = ipv4_specific;
 	subflow_specific.conn_request = subflow_v4_conn_request;
@@ -1407,7 +1419,7 @@ void __init mptcp_subflow_init(void)
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
 	subflow_request_sock_ipv6_ops = tcp_request_sock_ipv6_ops;
-	subflow_request_sock_ipv6_ops.init_req = subflow_v6_init_req;
+	subflow_request_sock_ipv6_ops.route_req = subflow_v6_route_req;
 
 	subflow_v6_specific = ipv6_specific;
 	subflow_v6_specific.conn_request = subflow_v6_conn_request;
-- 
2.26.2

[PATCH net-next 3/3] mptcp: emit tcp reset when a join request fails

[Florian Westphal]

RFC 8684 says:
 If the token is unknown or the host wants to refuse subflow establishment
 (for example, due to a limit on the number of subflows it will permit),
 the receiver will send back a reset (RST) signal, analogous to an unknown
 port in TCP, containing an MP_TCPRST option (Section 3.6) with an
 "MPTCP specific error" reason code.

mptcp-next doesn't support MP_TCPRST yet, this can be added in another
change.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/mptcp/subflow.c | 47 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c55b8f176746..5a8005746bc8 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -112,9 +112,14 @@ static int __subflow_init_req(struct request_sock *req, const struct sock *sk_li
 	return 0;
 }
 
-static void subflow_init_req(struct request_sock *req,
-			     const struct sock *sk_listener,
-			     struct sk_buff *skb)
+/* Init mptcp request socket.
+ *
+ * Returns an error code if a JOIN has failed and a TCP reset
+ * should be sent.
+ */
+static int subflow_init_req(struct request_sock *req,
+			    const struct sock *sk_listener,
+			    struct sk_buff *skb)
 {
 	struct mptcp_subflow_context *listener = mptcp_subflow_ctx(sk_listener);
 	struct mptcp_subflow_request_sock *subflow_req = mptcp_subflow_rsk(req);
@@ -125,7 +130,7 @@ static void subflow_init_req(struct request_sock *req,
 
 	ret = __subflow_init_req(req, sk_listener);
 	if (ret)
-		return;
+		return 0;
 
 	mptcp_get_options(skb, &mp_opt);
 
@@ -133,7 +138,7 @@ static void subflow_init_req(struct request_sock *req,
 		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVE);
 
 		if (mp_opt.mp_join)
-			return;
+			return 0;
 	} else if (mp_opt.mp_join) {
 		SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINSYNRX);
 	}
@@ -157,7 +162,7 @@ static void subflow_init_req(struct request_sock *req,
 			} else {
 				subflow_req->mp_capable = 1;
 			}
-			return;
+			return 0;
 		}
 
 		err = mptcp_token_new_request(req);
@@ -175,7 +180,11 @@ static void subflow_init_req(struct request_sock *req,
 		subflow_req->remote_nonce = mp_opt.nonce;
 		subflow_req->msk = subflow_token_join_request(req, skb);
 
-		if (unlikely(req->syncookie) && subflow_req->msk) {
+		/* Can't fall back to TCP in this case. */
+		if (!subflow_req->msk)
+			return -EPERM;
+
+		if (unlikely(req->syncookie)) {
 			if (mptcp_can_accept_new_subflow(subflow_req->msk))
 				subflow_init_req_cookie_join_save(subflow_req, skb);
 		}
@@ -183,6 +192,8 @@ static void subflow_init_req(struct request_sock *req,
 		pr_debug("token=%u, remote_nonce=%u msk=%p", subflow_req->token,
 			 subflow_req->remote_nonce, subflow_req->msk);
 	}
+
+	return 0;
 }
 
 int mptcp_subflow_init_cookie_req(struct request_sock *req,
@@ -234,6 +245,7 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 					      struct request_sock *req)
 {
 	struct dst_entry *dst;
+	int err;
 
 	tcp_rsk(req)->is_mptcp = 1;
 
@@ -241,8 +253,14 @@ static struct dst_entry *subflow_v4_route_req(const struct sock *sk,
 	if (!dst)
 		return NULL;
 
-	subflow_init_req(req, sk, skb);
-	return dst;
+	err = subflow_init_req(req, sk, skb);
+	if (err == 0)
+		return dst;
+
+	dst_release(dst);
+	if (!req->syncookie)
+		tcp_request_sock_ops.send_reset(sk, skb);
+	return NULL;
 }
 
 #if IS_ENABLED(CONFIG_MPTCP_IPV6)
@@ -252,6 +270,7 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 					      struct request_sock *req)
 {
 	struct dst_entry *dst;
+	int err;
 
 	tcp_rsk(req)->is_mptcp = 1;
 
@@ -259,8 +278,14 @@ static struct dst_entry *subflow_v6_route_req(const struct sock *sk,
 	if (!dst)
 		return NULL;
 
-	subflow_init_req(req, sk, skb);
-	return dst;
+	err = subflow_init_req(req, sk, skb);
+	if (err == 0)
+		return dst;
+
+	dst_release(dst);
+	if (!req->syncookie)
+		tcp6_request_sock_ops.send_reset(sk, skb);
+	return NULL;
 }
 #endif
 
-- 
2.26.2

Re: [MPTCP] [PATCH net-next 3/3] mptcp: emit tcp reset when a join request fails

[Mat Martineau]

On Mon, 30 Nov 2020, Florian Westphal wrote:

> RFC 8684 says:
> If the token is unknown or the host wants to refuse subflow establishment
> (for example, due to a limit on the number of subflows it will permit),
> the receiver will send back a reset (RST) signal, analogous to an unknown
> port in TCP, containing an MP_TCPRST option (Section 3.6) with an
> "MPTCP specific error" reason code.
>
> mptcp-next doesn't support MP_TCPRST yet, this can be added in another
> change.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
> net/mptcp/subflow.c | 47 ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 36 insertions(+), 11 deletions(-)
>

Thanks for the patch, Florian. Hopefully the first two in the series are 
ok with those maintainers, and for this one I can add:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel