* [PATCH net v5 2/3] net: prevent rewrite of msg_name in sock_sendmsg()
2023-09-21 23:46 [PATCH net v5 1/3] net: replace calls to sock->ops->connect() with kernel_connect() Jordan Rife
@ 2023-09-21 23:46 ` Jordan Rife
2023-09-27 5:50 ` Simon Horman
2023-09-21 23:46 ` [PATCH net v5 3/3] net: prevent address rewrite in kernel_bind() Jordan Rife
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Jordan Rife @ 2023-09-21 23:46 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, willemdebruijn.kernel, netdev
Cc: dborkman, horms, pablo, kadlec, fw, santosh.shilimkar, ast, rdna,
Jordan Rife, stable, Willem de Bruijn
Callers of sock_sendmsg(), and similarly kernel_sendmsg(), in kernel
space may observe their value of msg_name change in cases where BPF
sendmsg hooks rewrite the send address. This has been confirmed to break
NFS mounts running in UDP mode and has the potential to break other
systems.
This patch:
1) Creates a new function called __sock_sendmsg() with same logic as the
old sock_sendmsg() function.
2) Replaces calls to sock_sendmsg() made by __sys_sendto() and
__sys_sendmsg() with __sock_sendmsg() to avoid an unnecessary copy,
as these system calls are already protected.
3) Modifies sock_sendmsg() so that it makes a copy of msg_name if
present before passing it down the stack to insulate callers from
changes to the send address.
Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/
Fixes: 1cedee13d25a ("bpf: Hooks for sys_sendmsg")
Cc: stable@vger.kernel.org
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Jordan Rife <jrife@google.com>
---
v4->v5: Remove non-net changes.
v3->v4: Maintain reverse xmas tree order for variable declarations.
Remove precondition check for msg_namelen.
v2->v3: Add "Fixes" tag.
v1->v2: Split up original patch into patch series. Perform address copy
in sock_sendmsg() instead of sock->ops->sendmsg().
net/socket.c | 29 +++++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index c8b08b32f097e..a39ec136f5cff 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -737,6 +737,14 @@ static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg)
return ret;
}
+static int __sock_sendmsg(struct socket *sock, struct msghdr *msg)
+{
+ int err = security_socket_sendmsg(sock, msg,
+ msg_data_left(msg));
+
+ return err ?: sock_sendmsg_nosec(sock, msg);
+}
+
/**
* sock_sendmsg - send a message through @sock
* @sock: socket
@@ -747,10 +755,19 @@ static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg)
*/
int sock_sendmsg(struct socket *sock, struct msghdr *msg)
{
- int err = security_socket_sendmsg(sock, msg,
- msg_data_left(msg));
+ struct sockaddr_storage *save_addr = (struct sockaddr_storage *)msg->msg_name;
+ struct sockaddr_storage address;
+ int ret;
- return err ?: sock_sendmsg_nosec(sock, msg);
+ if (msg->msg_name) {
+ memcpy(&address, msg->msg_name, msg->msg_namelen);
+ msg->msg_name = &address;
+ }
+
+ ret = __sock_sendmsg(sock, msg);
+ msg->msg_name = save_addr;
+
+ return ret;
}
EXPORT_SYMBOL(sock_sendmsg);
@@ -1138,7 +1155,7 @@ static ssize_t sock_write_iter(struct kiocb *iocb, struct iov_iter *from)
if (sock->type == SOCK_SEQPACKET)
msg.msg_flags |= MSG_EOR;
- res = sock_sendmsg(sock, &msg);
+ res = __sock_sendmsg(sock, &msg);
*from = msg.msg_iter;
return res;
}
@@ -2174,7 +2191,7 @@ int __sys_sendto(int fd, void __user *buff, size_t len, unsigned int flags,
if (sock->file->f_flags & O_NONBLOCK)
flags |= MSG_DONTWAIT;
msg.msg_flags = flags;
- err = sock_sendmsg(sock, &msg);
+ err = __sock_sendmsg(sock, &msg);
out_put:
fput_light(sock->file, fput_needed);
@@ -2538,7 +2555,7 @@ static int ____sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
err = sock_sendmsg_nosec(sock, msg_sys);
goto out_freectl;
}
- err = sock_sendmsg(sock, msg_sys);
+ err = __sock_sendmsg(sock, msg_sys);
/*
* If this is sendmmsg() and sending to current destination address was
* successful, remember it.
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net v5 2/3] net: prevent rewrite of msg_name in sock_sendmsg()
2023-09-21 23:46 ` [PATCH net v5 2/3] net: prevent rewrite of msg_name in sock_sendmsg() Jordan Rife
@ 2023-09-27 5:50 ` Simon Horman
0 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2023-09-27 5:50 UTC (permalink / raw)
To: Jordan Rife
Cc: davem, edumazet, kuba, pabeni, willemdebruijn.kernel, netdev,
dborkman, pablo, kadlec, fw, santosh.shilimkar, ast, rdna, stable,
Willem de Bruijn
On Thu, Sep 21, 2023 at 06:46:41PM -0500, Jordan Rife wrote:
> Callers of sock_sendmsg(), and similarly kernel_sendmsg(), in kernel
> space may observe their value of msg_name change in cases where BPF
> sendmsg hooks rewrite the send address. This has been confirmed to break
> NFS mounts running in UDP mode and has the potential to break other
> systems.
>
> This patch:
>
> 1) Creates a new function called __sock_sendmsg() with same logic as the
> old sock_sendmsg() function.
> 2) Replaces calls to sock_sendmsg() made by __sys_sendto() and
> __sys_sendmsg() with __sock_sendmsg() to avoid an unnecessary copy,
> as these system calls are already protected.
> 3) Modifies sock_sendmsg() so that it makes a copy of msg_name if
> present before passing it down the stack to insulate callers from
> changes to the send address.
>
> Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/
> Fixes: 1cedee13d25a ("bpf: Hooks for sys_sendmsg")
> Cc: stable@vger.kernel.org
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Jordan Rife <jrife@google.com>
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net v5 3/3] net: prevent address rewrite in kernel_bind()
2023-09-21 23:46 [PATCH net v5 1/3] net: replace calls to sock->ops->connect() with kernel_connect() Jordan Rife
2023-09-21 23:46 ` [PATCH net v5 2/3] net: prevent rewrite of msg_name in sock_sendmsg() Jordan Rife
@ 2023-09-21 23:46 ` Jordan Rife
2023-09-27 5:51 ` Simon Horman
2023-09-27 5:50 ` [PATCH net v5 1/3] net: replace calls to sock->ops->connect() with kernel_connect() Simon Horman
2023-10-01 18:40 ` patchwork-bot+netdevbpf
3 siblings, 1 reply; 7+ messages in thread
From: Jordan Rife @ 2023-09-21 23:46 UTC (permalink / raw)
To: davem, edumazet, kuba, pabeni, willemdebruijn.kernel, netdev
Cc: dborkman, horms, pablo, kadlec, fw, santosh.shilimkar, ast, rdna,
Jordan Rife, stable, Willem de Bruijn
Similar to the change in commit 0bdf399342c5("net: Avoid address
overwrite in kernel_connect"), BPF hooks run on bind may rewrite the
address passed to kernel_bind(). This change
1) Makes a copy of the bind address in kernel_bind() to insulate
callers.
2) Replaces direct calls to sock->ops->bind() in net with kernel_bind()
Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/
Fixes: 4fbac77d2d09 ("bpf: Hooks for sys_bind")
Cc: stable@vger.kernel.org
Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Jordan Rife <jrife@google.com>
---
v4->v5: Remove non-net changes.
v3->v4: Remove precondition check for addrlen. Pass address copy to
bind() instead of original address.
v2->v3: Add "Fixes" tag. Check for positivity in addrlen sanity check.
v1->v2: Split up original patch into patch series. Insulate
sock->ops->bind() calls with kernel_bind().
net/netfilter/ipvs/ip_vs_sync.c | 4 ++--
net/rds/tcp_connect.c | 2 +-
net/rds/tcp_listen.c | 2 +-
net/socket.c | 7 ++++++-
4 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 6e4ed1e11a3b7..4174076c66fa7 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -1439,7 +1439,7 @@ static int bind_mcastif_addr(struct socket *sock, struct net_device *dev)
sin.sin_addr.s_addr = addr;
sin.sin_port = 0;
- return sock->ops->bind(sock, (struct sockaddr*)&sin, sizeof(sin));
+ return kernel_bind(sock, (struct sockaddr *)&sin, sizeof(sin));
}
static void get_mcast_sockaddr(union ipvs_sockaddr *sa, int *salen,
@@ -1546,7 +1546,7 @@ static int make_receive_sock(struct netns_ipvs *ipvs, int id,
get_mcast_sockaddr(&mcast_addr, &salen, &ipvs->bcfg, id);
sock->sk->sk_bound_dev_if = dev->ifindex;
- result = sock->ops->bind(sock, (struct sockaddr *)&mcast_addr, salen);
+ result = kernel_bind(sock, (struct sockaddr *)&mcast_addr, salen);
if (result < 0) {
pr_err("Error binding to the multicast addr\n");
goto error;
diff --git a/net/rds/tcp_connect.c b/net/rds/tcp_connect.c
index d788c6d28986f..a0046e99d6df7 100644
--- a/net/rds/tcp_connect.c
+++ b/net/rds/tcp_connect.c
@@ -145,7 +145,7 @@ int rds_tcp_conn_path_connect(struct rds_conn_path *cp)
addrlen = sizeof(sin);
}
- ret = sock->ops->bind(sock, addr, addrlen);
+ ret = kernel_bind(sock, addr, addrlen);
if (ret) {
rdsdebug("bind failed with %d at address %pI6c\n",
ret, &conn->c_laddr);
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 014fa24418c12..53b3535a1e4a8 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -306,7 +306,7 @@ struct socket *rds_tcp_listen_init(struct net *net, bool isv6)
addr_len = sizeof(*sin);
}
- ret = sock->ops->bind(sock, (struct sockaddr *)&ss, addr_len);
+ ret = kernel_bind(sock, (struct sockaddr *)&ss, addr_len);
if (ret < 0) {
rdsdebug("could not bind %s listener socket: %d\n",
isv6 ? "IPv6" : "IPv4", ret);
diff --git a/net/socket.c b/net/socket.c
index a39ec136f5cff..c4a6f55329552 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3516,7 +3516,12 @@ static long compat_sock_ioctl(struct file *file, unsigned int cmd,
int kernel_bind(struct socket *sock, struct sockaddr *addr, int addrlen)
{
- return READ_ONCE(sock->ops)->bind(sock, addr, addrlen);
+ struct sockaddr_storage address;
+
+ memcpy(&address, addr, addrlen);
+
+ return READ_ONCE(sock->ops)->bind(sock, (struct sockaddr *)&address,
+ addrlen);
}
EXPORT_SYMBOL(kernel_bind);
--
2.42.0.515.g380fc7ccd1-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH net v5 3/3] net: prevent address rewrite in kernel_bind()
2023-09-21 23:46 ` [PATCH net v5 3/3] net: prevent address rewrite in kernel_bind() Jordan Rife
@ 2023-09-27 5:51 ` Simon Horman
0 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2023-09-27 5:51 UTC (permalink / raw)
To: Jordan Rife
Cc: davem, edumazet, kuba, pabeni, willemdebruijn.kernel, netdev,
dborkman, pablo, kadlec, fw, santosh.shilimkar, ast, rdna, stable,
Willem de Bruijn
On Thu, Sep 21, 2023 at 06:46:42PM -0500, Jordan Rife wrote:
> Similar to the change in commit 0bdf399342c5("net: Avoid address
> overwrite in kernel_connect"), BPF hooks run on bind may rewrite the
> address passed to kernel_bind(). This change
>
> 1) Makes a copy of the bind address in kernel_bind() to insulate
> callers.
> 2) Replaces direct calls to sock->ops->bind() in net with kernel_bind()
>
> Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/
> Fixes: 4fbac77d2d09 ("bpf: Hooks for sys_bind")
> Cc: stable@vger.kernel.org
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Jordan Rife <jrife@google.com>
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net v5 1/3] net: replace calls to sock->ops->connect() with kernel_connect()
2023-09-21 23:46 [PATCH net v5 1/3] net: replace calls to sock->ops->connect() with kernel_connect() Jordan Rife
2023-09-21 23:46 ` [PATCH net v5 2/3] net: prevent rewrite of msg_name in sock_sendmsg() Jordan Rife
2023-09-21 23:46 ` [PATCH net v5 3/3] net: prevent address rewrite in kernel_bind() Jordan Rife
@ 2023-09-27 5:50 ` Simon Horman
2023-10-01 18:40 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 7+ messages in thread
From: Simon Horman @ 2023-09-27 5:50 UTC (permalink / raw)
To: Jordan Rife
Cc: davem, edumazet, kuba, pabeni, willemdebruijn.kernel, netdev,
dborkman, pablo, kadlec, fw, santosh.shilimkar, ast, rdna, stable,
Willem de Bruijn
On Thu, Sep 21, 2023 at 06:46:40PM -0500, Jordan Rife wrote:
> commit 0bdf399342c5 ("net: Avoid address overwrite in kernel_connect")
> ensured that kernel_connect() will not overwrite the address parameter
> in cases where BPF connect hooks perform an address rewrite. This change
> replaces direct calls to sock->ops->connect() in net with kernel_connect()
> to make these call safe.
>
> Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/
> Fixes: d74bad4e74ee ("bpf: Hooks for sys_connect")
> Cc: stable@vger.kernel.org
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Jordan Rife <jrife@google.com>
Reviewed-by: Simon Horman <horms@kernel.org>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net v5 1/3] net: replace calls to sock->ops->connect() with kernel_connect()
2023-09-21 23:46 [PATCH net v5 1/3] net: replace calls to sock->ops->connect() with kernel_connect() Jordan Rife
` (2 preceding siblings ...)
2023-09-27 5:50 ` [PATCH net v5 1/3] net: replace calls to sock->ops->connect() with kernel_connect() Simon Horman
@ 2023-10-01 18:40 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-10-01 18:40 UTC (permalink / raw)
To: Jordan Rife
Cc: davem, edumazet, kuba, pabeni, willemdebruijn.kernel, netdev,
dborkman, horms, pablo, kadlec, fw, santosh.shilimkar, ast, rdna,
stable, willemb
Hello:
This series was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:
On Thu, 21 Sep 2023 18:46:40 -0500 you wrote:
> commit 0bdf399342c5 ("net: Avoid address overwrite in kernel_connect")
> ensured that kernel_connect() will not overwrite the address parameter
> in cases where BPF connect hooks perform an address rewrite. This change
> replaces direct calls to sock->ops->connect() in net with kernel_connect()
> to make these call safe.
>
> Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/
> Fixes: d74bad4e74ee ("bpf: Hooks for sys_connect")
> Cc: stable@vger.kernel.org
> Reviewed-by: Willem de Bruijn <willemb@google.com>
> Signed-off-by: Jordan Rife <jrife@google.com>
>
> [...]
Here is the summary with links:
- [net,v5,1/3] net: replace calls to sock->ops->connect() with kernel_connect()
https://git.kernel.org/netdev/net/c/26297b4ce1ce
- [net,v5,2/3] net: prevent rewrite of msg_name in sock_sendmsg()
https://git.kernel.org/netdev/net/c/86a7e0b69bd5
- [net,v5,3/3] net: prevent address rewrite in kernel_bind()
https://git.kernel.org/netdev/net/c/c889a99a21bf
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 7+ messages in thread