[PATCH] net: Added security socket

[PATCH] net: Added security socket

[Denis Arefev]

	Added security_socket_connect
	kernel_connect is in kernel space,
	but kernel_connect is used in RPC 
	requests (/net/sunrpc/xprtsock.c),  
	and the RPC protocol is used by the NFS server.
	This is how we protect the TCP connection 
	initiated by the client. 

Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 net/socket.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/socket.c b/net/socket.c
index 9c92c0e6c4da..9afa2b44a9e5 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3526,6 +3526,12 @@ EXPORT_SYMBOL(kernel_accept);
 int kernel_connect(struct socket *sock, struct sockaddr *addr, int addrlen,
 		   int flags)
 {
+	int err;
+
+	err = security_socket_connect(sock, (struct sockaddr *)addr, addrlen);
+	if (err)
+		return err;
+
 	return sock->ops->connect(sock, addr, addrlen, flags);
 }
 EXPORT_SYMBOL(kernel_connect);
-- 
2.25.1

Re: [PATCH] net: Added security socket

[Jakub Kicinski]

On Wed,  5 Apr 2023 15:53:08 +0300 Denis Arefev wrote:
> 	Added security_socket_connect
> 	kernel_connect is in kernel space,
> 	but kernel_connect is used in RPC 
> 	requests (/net/sunrpc/xprtsock.c),  
> 	and the RPC protocol is used by the NFS server.
> 	This is how we protect the TCP connection 
> 	initiated by the client. 

Can you please format this to look like every other commit in the
kernel and use imperative mood?

Then please add to the description _exactly_ how you're going to use
it, i.e. an example of a real rule. And CC
linux-security-module@vger.kernel.org

[PATCH] net: Added security socket

[Denis Arefev]

	Added security_socket_connect
	in kernel_connect

Signed-off-by: Denis Arefev <arefev@swemel.ru>
---
 net/socket.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/socket.c b/net/socket.c
index 9c92c0e6c4da..9afa2b44a9e5 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3526,6 +3526,12 @@ EXPORT_SYMBOL(kernel_accept);
 int kernel_connect(struct socket *sock, struct sockaddr *addr, int addrlen,
 		   int flags)
 {
+	int err;
+
+	err = security_socket_connect(sock, (struct sockaddr *)addr, addrlen);
+	if (err)
+		return err;
+
 	return sock->ops->connect(sock, addr, addrlen, flags);
 }
 EXPORT_SYMBOL(kernel_connect);
-- 
2.25.1

Re: [PATCH] net: Added security socket

[Alexander H Duyck]

On Mon, 2023-04-03 at 15:43 +0300, Denis Arefev wrote:
> 	Added security_socket_connect
> 	in kernel_connect
> 
> Signed-off-by: Denis Arefev <arefev@swemel.ru>
> ---
>  net/socket.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/socket.c b/net/socket.c
> index 9c92c0e6c4da..9afa2b44a9e5 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -3526,6 +3526,12 @@ EXPORT_SYMBOL(kernel_accept);
>  int kernel_connect(struct socket *sock, struct sockaddr *addr, int addrlen,
>  		   int flags)
>  {
> +	int err;
> +
> +	err = security_socket_connect(sock, (struct sockaddr *)addr, addrlen);
> +	if (err)
> +		return err;
> +
>  	return sock->ops->connect(sock, addr, addrlen, flags);
>  }
>  EXPORT_SYMBOL(kernel_connect);

Why would we need to be adding this? If we are already operating within
kernel space it seems like it would be more problematic than not to
have to push items out to userspace for security. Assuming an attacker
is operating at the kernel level the system is already compromised is
it not?

Also assuming we do need this why are we only dealing with connect when
we should probably also be looking at all the other kernel socket calls
then as well?

[PATCH] net: Added security socket

[Denis Arefev]

Hi Alexander. I understand your concern. 
That's right kernel_connect is in kernel space,
but kernel_connect is used in RPC requests (/net/sunrpc/xprtsock.c),
and the RPC protocol is used by the NFS server.
Note kernel_sendmsg is already protected.

Re: [PATCH] net: Added security socket

[Alexander Duyck]

On Tue, Apr 4, 2023 at 1:00 AM Denis Arefev <arefev@swemel.ru> wrote:
>
> Hi Alexander. I understand your concern.
> That's right kernel_connect is in kernel space,
> but kernel_connect is used in RPC requests (/net/sunrpc/xprtsock.c),
> and the RPC protocol is used by the NFS server.
> Note kernel_sendmsg is already protected.

Can you add a write-up about the need for this in the patch
description? Your patch description described what you are doing but
not the why. My main concern is that your patch may end up causing
issues that could later be reverted by someone if they don't
understand the motivation behind why you are making this change.

Calling out things like the fact that you are trying to add security
to RPC sockets would be useful and would make it easier for patch
review as people more familiar with the security code could also tell
you if this is the correct approach to this or not.

Thanks,

- Alex