[PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[Xin Long]

Now when using 'ss' in iproute, kernel would try to load all _diag
modules. It causes the corresponding family or proto modules to be
loaded as well.

Like after 'ss -a', sctp, dccp, af_packet(if it works as a moudle)
will be loaded.

As these family or proto modules are loaded unexpectly, this might
have some security implications.

This patch is to introduce sock_diag_request_module() in which we
only request the _diag module when it's corresponding family or
proto has been registered.

Note that we can't just load _diag module without the family or
proto loaded, as some symbols in _diag module are from the family
or proto moudle.

Reported-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 include/linux/net.h       |  1 +
 include/linux/sock_diag.h |  1 +
 net/core/sock_diag.c      | 32 ++++++++++++++++++++++++--------
 net/ipv4/inet_diag.c      |  3 +--
 net/socket.c              |  5 +++++
 5 files changed, 32 insertions(+), 10 deletions(-)

diff --git a/include/linux/net.h b/include/linux/net.h
index d97d80d..6c7cf09 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -225,6 +225,7 @@ enum {
 int sock_wake_async(struct socket_wq *sk_wq, int how, int band);
 int sock_register(const struct net_proto_family *fam);
 void sock_unregister(int family);
+bool sock_is_registered(int family);
 int __sock_create(struct net *net, int family, int type, int proto,
 		  struct socket **res, int kern);
 int sock_create(int family, int type, int proto, struct socket **res);
diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h
index a2f8109..e9a1b8d 100644
--- a/include/linux/sock_diag.h
+++ b/include/linux/sock_diag.h
@@ -77,4 +77,5 @@ bool sock_diag_has_destroy_listeners(const struct sock *sk)
 void sock_diag_broadcast_destroy(struct sock *sk);
 
 int sock_diag_destroy(struct sock *sk, int err);
+int sock_diag_request_module(int family, int protocol);
 #endif
diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index 217f4e3..caad6b6 100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c
@@ -7,6 +7,7 @@
 #include <net/net_namespace.h>
 #include <linux/module.h>
 #include <net/sock.h>
+#include <net/protocol.h>
 #include <linux/kernel.h>
 #include <linux/tcp.h>
 #include <linux/workqueue.h>
@@ -207,6 +208,25 @@ void sock_diag_unregister(const struct sock_diag_handler *hnld)
 }
 EXPORT_SYMBOL_GPL(sock_diag_unregister);
 
+int sock_diag_request_module(int family, int protocol)
+{
+	if (!protocol) {
+		if (!sock_is_registered(family))
+			return -ENOENT;
+
+		return request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
+				      NETLINK_SOCK_DIAG, family);
+	}
+
+	if (family == AF_INET &&
+	    !rcu_access_pointer(inet_protos[protocol]))
+		return -ENOENT;
+
+	return request_module("net-pf-%d-proto-%d-type-%d-%d", PF_NETLINK,
+			      NETLINK_SOCK_DIAG, family, protocol);
+}
+EXPORT_SYMBOL_GPL(sock_diag_request_module);
+
 static int __sock_diag_cmd(struct sk_buff *skb, struct nlmsghdr *nlh)
 {
 	int err;
@@ -220,8 +240,7 @@ static int __sock_diag_cmd(struct sk_buff *skb, struct nlmsghdr *nlh)
 		return -EINVAL;
 
 	if (sock_diag_handlers[req->sdiag_family] == NULL)
-		request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-				NETLINK_SOCK_DIAG, req->sdiag_family);
+		sock_diag_request_module(req->sdiag_family, 0);
 
 	mutex_lock(&sock_diag_table_mutex);
 	hndl = sock_diag_handlers[req->sdiag_family];
@@ -247,8 +266,7 @@ static int sock_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
 	case TCPDIAG_GETSOCK:
 	case DCCPDIAG_GETSOCK:
 		if (inet_rcv_compat == NULL)
-			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-					NETLINK_SOCK_DIAG, AF_INET);
+			sock_diag_request_module(AF_INET, 0);
 
 		mutex_lock(&sock_diag_table_mutex);
 		if (inet_rcv_compat != NULL)
@@ -281,14 +299,12 @@ static int sock_diag_bind(struct net *net, int group)
 	case SKNLGRP_INET_TCP_DESTROY:
 	case SKNLGRP_INET_UDP_DESTROY:
 		if (!sock_diag_handlers[AF_INET])
-			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-				       NETLINK_SOCK_DIAG, AF_INET);
+			sock_diag_request_module(AF_INET, 0);
 		break;
 	case SKNLGRP_INET6_TCP_DESTROY:
 	case SKNLGRP_INET6_UDP_DESTROY:
 		if (!sock_diag_handlers[AF_INET6])
-			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
-				       NETLINK_SOCK_DIAG, AF_INET);
+			sock_diag_request_module(AF_INET, 0);
 		break;
 	}
 	return 0;
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index c9c35b6..d10c8fa 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -53,8 +53,7 @@ static DEFINE_MUTEX(inet_diag_table_mutex);
 static const struct inet_diag_handler *inet_diag_lock_handler(int proto)
 {
 	if (!inet_diag_table[proto])
-		request_module("net-pf-%d-proto-%d-type-%d-%d", PF_NETLINK,
-			       NETLINK_SOCK_DIAG, AF_INET, proto);
+		sock_diag_request_module(AF_INET, proto);
 
 	mutex_lock(&inet_diag_table_mutex);
 	if (!inet_diag_table[proto])
diff --git a/net/socket.c b/net/socket.c
index c729625..733d657 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -2590,6 +2590,11 @@ void sock_unregister(int family)
 }
 EXPORT_SYMBOL(sock_unregister);
 
+bool sock_is_registered(int family)
+{
+	return family < NPROTO && rcu_access_pointer(net_families[family]);
+}
+
 static int __init sock_init(void)
 {
 	int err;
-- 
2.1.0

Re: [PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[Eric Dumazet]

On Tue, 2017-10-17 at 13:13 +0800, Xin Long wrote:

> +bool sock_is_registered(int family)
> +{
> +	return family < NPROTO && rcu_access_pointer(net_families[family]);
> +}

EXPORT_SYMBOL() ?

Re: [PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[Eric Dumazet]

On Tue, 2017-10-17 at 04:16 -0700, Eric Dumazet wrote:
> On Tue, 2017-10-17 at 13:13 +0800, Xin Long wrote:
> 
> > +bool sock_is_registered(int family)
> > +{
> > +	return family < NPROTO && rcu_access_pointer(net_families[family]);
> > +}
> 
> EXPORT_SYMBOL() ?
> 

Or move it in net/core/sock_diag.c so that it can be static.

Re: [PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[Eric Dumazet]

On Tue, 2017-10-17 at 04:16 -0700, Eric Dumazet wrote:
> On Tue, 2017-10-17 at 13:13 +0800, Xin Long wrote:
> 
> > +bool sock_is_registered(int family)
> > +{
> > +	return family < NPROTO && rcu_access_pointer(net_families[family]);
> > +}
> 
> EXPORT_SYMBOL() ?

I take this back. Since there is one single user in vmlinux, no need yet

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[Marcelo Ricardo Leitner]

On Tue, Oct 17, 2017 at 01:13:56PM +0800, Xin Long wrote:
> Now when using 'ss' in iproute, kernel would try to load all _diag
> modules. It causes the corresponding family or proto modules to be
> loaded as well.
> 
> Like after 'ss -a', sctp, dccp, af_packet(if it works as a moudle)
> will be loaded.
> 
> As these family or proto modules are loaded unexpectly, this might
> have some security implications.
> 
> This patch is to introduce sock_diag_request_module() in which we
> only request the _diag module when it's corresponding family or
> proto has been registered.
> 
> Note that we can't just load _diag module without the family or
> proto loaded, as some symbols in _diag module are from the family
> or proto moudle.
> 
> Reported-by: Sabrina Dubroca <sd@queasysnail.net>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>

Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

> ---
>  include/linux/net.h       |  1 +
>  include/linux/sock_diag.h |  1 +
>  net/core/sock_diag.c      | 32 ++++++++++++++++++++++++--------
>  net/ipv4/inet_diag.c      |  3 +--
>  net/socket.c              |  5 +++++
>  5 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/net.h b/include/linux/net.h
> index d97d80d..6c7cf09 100644
> --- a/include/linux/net.h
> +++ b/include/linux/net.h
> @@ -225,6 +225,7 @@ enum {
>  int sock_wake_async(struct socket_wq *sk_wq, int how, int band);
>  int sock_register(const struct net_proto_family *fam);
>  void sock_unregister(int family);
> +bool sock_is_registered(int family);
>  int __sock_create(struct net *net, int family, int type, int proto,
>  		  struct socket **res, int kern);
>  int sock_create(int family, int type, int proto, struct socket **res);
> diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h
> index a2f8109..e9a1b8d 100644
> --- a/include/linux/sock_diag.h
> +++ b/include/linux/sock_diag.h
> @@ -77,4 +77,5 @@ bool sock_diag_has_destroy_listeners(const struct sock *sk)
>  void sock_diag_broadcast_destroy(struct sock *sk);
>  
>  int sock_diag_destroy(struct sock *sk, int err);
> +int sock_diag_request_module(int family, int protocol);
>  #endif
> diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
> index 217f4e3..caad6b6 100644
> --- a/net/core/sock_diag.c
> +++ b/net/core/sock_diag.c
> @@ -7,6 +7,7 @@
>  #include <net/net_namespace.h>
>  #include <linux/module.h>
>  #include <net/sock.h>
> +#include <net/protocol.h>
>  #include <linux/kernel.h>
>  #include <linux/tcp.h>
>  #include <linux/workqueue.h>
> @@ -207,6 +208,25 @@ void sock_diag_unregister(const struct sock_diag_handler *hnld)
>  }
>  EXPORT_SYMBOL_GPL(sock_diag_unregister);
>  
> +int sock_diag_request_module(int family, int protocol)
> +{
> +	if (!protocol) {
> +		if (!sock_is_registered(family))
> +			return -ENOENT;
> +
> +		return request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
> +				      NETLINK_SOCK_DIAG, family);
> +	}
> +
> +	if (family == AF_INET &&
> +	    !rcu_access_pointer(inet_protos[protocol]))
> +		return -ENOENT;
> +
> +	return request_module("net-pf-%d-proto-%d-type-%d-%d", PF_NETLINK,
> +			      NETLINK_SOCK_DIAG, family, protocol);
> +}
> +EXPORT_SYMBOL_GPL(sock_diag_request_module);
> +
>  static int __sock_diag_cmd(struct sk_buff *skb, struct nlmsghdr *nlh)
>  {
>  	int err;
> @@ -220,8 +240,7 @@ static int __sock_diag_cmd(struct sk_buff *skb, struct nlmsghdr *nlh)
>  		return -EINVAL;
>  
>  	if (sock_diag_handlers[req->sdiag_family] == NULL)
> -		request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
> -				NETLINK_SOCK_DIAG, req->sdiag_family);
> +		sock_diag_request_module(req->sdiag_family, 0);
>  
>  	mutex_lock(&sock_diag_table_mutex);
>  	hndl = sock_diag_handlers[req->sdiag_family];
> @@ -247,8 +266,7 @@ static int sock_diag_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	case TCPDIAG_GETSOCK:
>  	case DCCPDIAG_GETSOCK:
>  		if (inet_rcv_compat == NULL)
> -			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
> -					NETLINK_SOCK_DIAG, AF_INET);
> +			sock_diag_request_module(AF_INET, 0);
>  
>  		mutex_lock(&sock_diag_table_mutex);
>  		if (inet_rcv_compat != NULL)
> @@ -281,14 +299,12 @@ static int sock_diag_bind(struct net *net, int group)
>  	case SKNLGRP_INET_TCP_DESTROY:
>  	case SKNLGRP_INET_UDP_DESTROY:
>  		if (!sock_diag_handlers[AF_INET])
> -			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
> -				       NETLINK_SOCK_DIAG, AF_INET);
> +			sock_diag_request_module(AF_INET, 0);
>  		break;
>  	case SKNLGRP_INET6_TCP_DESTROY:
>  	case SKNLGRP_INET6_UDP_DESTROY:
>  		if (!sock_diag_handlers[AF_INET6])
> -			request_module("net-pf-%d-proto-%d-type-%d", PF_NETLINK,
> -				       NETLINK_SOCK_DIAG, AF_INET);
> +			sock_diag_request_module(AF_INET, 0);
>  		break;
>  	}
>  	return 0;
> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index c9c35b6..d10c8fa 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -53,8 +53,7 @@ static DEFINE_MUTEX(inet_diag_table_mutex);
>  static const struct inet_diag_handler *inet_diag_lock_handler(int proto)
>  {
>  	if (!inet_diag_table[proto])
> -		request_module("net-pf-%d-proto-%d-type-%d-%d", PF_NETLINK,
> -			       NETLINK_SOCK_DIAG, AF_INET, proto);
> +		sock_diag_request_module(AF_INET, proto);
>  
>  	mutex_lock(&inet_diag_table_mutex);
>  	if (!inet_diag_table[proto])
> diff --git a/net/socket.c b/net/socket.c
> index c729625..733d657 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -2590,6 +2590,11 @@ void sock_unregister(int family)
>  }
>  EXPORT_SYMBOL(sock_unregister);
>  
> +bool sock_is_registered(int family)
> +{
> +	return family < NPROTO && rcu_access_pointer(net_families[family]);
> +}
> +
>  static int __init sock_init(void)
>  {
>  	int err;
> -- 
> 2.1.0
> 

Re: [PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 731 bytes --]

Hi Xin,

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Xin-Long/sock_diag-request-_diag-module-only-when-the-family-or-proto-has-been-registered/20171018-152434
config: i386-randconfig-a0-201742 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   net/core/sock_diag.o: In function `sock_diag_request_module':
>> sock_diag.c:(.text+0x5a5): undefined reference to `inet_protos'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24738 bytes --]

Re: [PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

Hi Xin,

[auto build test ERROR on net/master]

url:    https://github.com/0day-ci/linux/commits/Xin-Long/sock_diag-request-_diag-module-only-when-the-family-or-proto-has-been-registered/20171018-152434
config: x86_64-randconfig-a0-10181611 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   net/core/sock_diag.o: In function `sock_diag_request_module':
>> (.text+0x685): undefined reference to `inet_protos'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 25418 bytes --]

Re: [PATCH net] sock_diag: request _diag module only when the family or proto has been registered

[Xin Long]

On Wed, Oct 18, 2017 at 5:38 PM, kbuild test robot <lkp@intel.com> wrote:
> Hi Xin,
>
> [auto build test ERROR on net/master]
>
> url:    https://github.com/0day-ci/linux/commits/Xin-Long/sock_diag-request-_diag-module-only-when-the-family-or-proto-has-been-registered/20171018-152434
> config: x86_64-randconfig-a0-10181611 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64
>
> All errors (new ones prefixed by >>):
>
>    net/core/sock_diag.o: In function `sock_diag_request_module':
>>> (.text+0x685): undefined reference to `inet_protos'
Huh,
# CONFIG_INET is not set
Cool, I will move the check for inet_protos to net_diag.ko
and repost.

Thanks.


>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation