From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Jordan Rife <jrife@google.com>,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, netdev@vger.kernel.org
Cc: dborkman@kernel.org, Jordan Rife <jrife@google.com>
Subject: Re: [PATCH net] net: prevent address overwrite in connect() and sendmsg()
Date: Tue, 12 Sep 2023 09:33:05 -0400 [thread overview]
Message-ID: <65006891779ed_25e754294b8@willemb.c.googlers.com.notmuch> (raw)
In-Reply-To: <20230912013332.2048422-1-jrife@google.com>
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. However,
> there remain other cases where BPF hooks can overwrite an address held
> by a kernel client.
>
> ==Scenarios Tested==
>
> * Code in the SMB and Ceph modules calls sock->ops->connect() directly,
> allowing the address overwrite to occur. In the case of SMB, this can
> lead to broken mounts.
These should probably call kernel_connect instead.
> * NFS v3 mounts with proto=udp call sock_sendmsg() for each RPC call,
> passing a pointer to the mount address in msg->msg_name which is
> later overwritten by a BPF sendmsg hook. This can lead to broken NFS
> mounts.
Similarly, this could call kernel_sendmsg, and the extra copy handled
in that wrapper. The arguments are not exacty the same, so not 100%
this is feasible.
But it's preferable if in-kernel callers use the kernel_.. API rather
than bypass it. Exactly for issues like the one you report.
> In order to more comprehensively fix this class of problems, this patch
> pushes the address copy deeper into the stack and introduces an address
> copy to both udp_sendmsg() and udpv6_sendmsg() to insulate all callers
> from address rewrites.
>
> Signed-off-by: Jordan Rife <jrife@google.com>
> ---
> net/ipv4/af_inet.c | 18 ++++++++++++++++++
> net/ipv4/udp.c | 21 ++++++++++++++++-----
> net/ipv6/udp.c | 23 +++++++++++++++++------
> net/socket.c | 7 +------
> 4 files changed, 52 insertions(+), 17 deletions(-)
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 3d2e30e204735..c37d484fbee34 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -568,6 +568,7 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr,
> {
> struct sock *sk = sock->sk;
> const struct proto *prot;
> + struct sockaddr_storage addr;
> int err;
>
> if (addr_len < sizeof(uaddr->sa_family))
> @@ -580,6 +581,14 @@ int inet_dgram_connect(struct socket *sock, struct sockaddr *uaddr,
> return prot->disconnect(sk, flags);
>
> if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) {
> + if (uaddr && addr_len <= sizeof(addr)) {
> + /* pre_connect can rewrite uaddr, so make a copy to
> + * insulate the caller.
> + */
> + memcpy(&addr, uaddr, addr_len);
> + uaddr = (struct sockaddr *)&addr;
> + }
> +
> err = prot->pre_connect(sk, uaddr, addr_len);
> if (err)
> return err;
> @@ -625,6 +634,7 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> int addr_len, int flags, int is_sendmsg)
> {
> struct sock *sk = sock->sk;
> + struct sockaddr_storage addr;
> int err;
> long timeo;
>
> @@ -668,6 +678,14 @@ int __inet_stream_connect(struct socket *sock, struct sockaddr *uaddr,
> goto out;
>
> if (BPF_CGROUP_PRE_CONNECT_ENABLED(sk)) {
> + if (uaddr && addr_len <= sizeof(addr)) {
> + /* pre_connect can rewrite uaddr, so make a copy to
> + * insulate the caller.
> + */
> + memcpy(&addr, uaddr, addr_len);
> + uaddr = (struct sockaddr *)&addr;
> + }
> +
> err = sk->sk_prot->pre_connect(sk, uaddr, addr_len);
> if (err)
> goto out;
> diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> index f39b9c8445808..5f5ee2752eeb7 100644
> --- a/net/ipv4/udp.c
> +++ b/net/ipv4/udp.c
> @@ -1142,18 +1142,29 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> }
>
> if (cgroup_bpf_enabled(CGROUP_UDP4_SENDMSG) && !connected) {
> + struct sockaddr_in tmp_addr;
> + struct sockaddr_in *addr = usin;
> +
> + /* BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK can rewrite usin, so make a
> + * copy to insulate the caller.
> + */
> + if (usin && msg->msg_namelen <= sizeof(tmp_addr)) {
> + memcpy(&tmp_addr, usin, msg->msg_namelen);
> + addr = &tmp_addr;
> + }
> +
> err = BPF_CGROUP_RUN_PROG_UDP4_SENDMSG_LOCK(sk,
> - (struct sockaddr *)usin, &ipc.addr);
> + (struct sockaddr *)addr, &ipc.addr);
> if (err)
> goto out_free;
> - if (usin) {
> - if (usin->sin_port == 0) {
> + if (addr) {
> + if (addr->sin_port == 0) {
> /* BPF program set invalid port. Reject it. */
> err = -EINVAL;
> goto out_free;
> }
> - daddr = usin->sin_addr.s_addr;
> - dport = usin->sin_port;
> + daddr = addr->sin_addr.s_addr;
> + dport = addr->sin_port;
> }
> }
>
> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 86b5d509a4688..cbc1917fad629 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1506,26 +1506,37 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
> fl6->fl6_sport = inet->inet_sport;
>
> if (cgroup_bpf_enabled(CGROUP_UDP6_SENDMSG) && !connected) {
> + struct sockaddr_in6 tmp_addr;
> + struct sockaddr_in6 *addr = sin6;
> +
> + /* BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK can rewrite sin6, so make a
> + * copy to insulate the caller.
> + */
> + if (sin6 && addr_len <= sizeof(tmp_addr)) {
> + memcpy(&tmp_addr, sin6, addr_len);
> + addr = &tmp_addr;
> + }
> +
> err = BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK(sk,
> - (struct sockaddr *)sin6,
> + (struct sockaddr *)addr,
> &fl6->saddr);
> if (err)
> goto out_no_dst;
> - if (sin6) {
> - if (ipv6_addr_v4mapped(&sin6->sin6_addr)) {
> + if (addr) {
> + if (ipv6_addr_v4mapped(&addr->sin6_addr)) {
> /* BPF program rewrote IPv6-only by IPv4-mapped
> * IPv6. It's currently unsupported.
> */
> err = -ENOTSUPP;
> goto out_no_dst;
> }
> - if (sin6->sin6_port == 0) {
> + if (addr->sin6_port == 0) {
> /* BPF program set invalid port. Reject it. */
> err = -EINVAL;
> goto out_no_dst;
> }
> - fl6->fl6_dport = sin6->sin6_port;
> - fl6->daddr = sin6->sin6_addr;
> + fl6->fl6_dport = addr->sin6_port;
> + fl6->daddr = addr->sin6_addr;
> }
> }
>
> diff --git a/net/socket.c b/net/socket.c
> index c8b08b32f097e..39794d026fa11 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -3570,12 +3570,7 @@ EXPORT_SYMBOL(kernel_accept);
> int kernel_connect(struct socket *sock, struct sockaddr *addr, int addrlen,
> int flags)
> {
> - struct sockaddr_storage address;
> -
> - memcpy(&address, addr, addrlen);
> -
> - return READ_ONCE(sock->ops)->connect(sock, (struct sockaddr *)&address,
> - addrlen, flags);
> + return READ_ONCE(sock->ops)->connect(sock, addr, addrlen, flags);
> }
> EXPORT_SYMBOL(kernel_connect);
>
> --
> 2.42.0.283.g2d96d420d3-goog
>
next prev parent reply other threads:[~2023-09-12 13:33 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-12 1:33 [PATCH net] net: prevent address overwrite in connect() and sendmsg() Jordan Rife
2023-09-12 13:33 ` Willem de Bruijn [this message]
2023-09-12 14:22 ` Daniel Borkmann
2023-09-12 18:31 ` Jordan Rife
2023-09-12 19:35 ` Willem de Bruijn
2023-09-12 20:07 ` Jordan Rife
2023-09-12 20:48 ` Willem de Bruijn
2023-09-12 21:08 ` Jordan Rife
2023-09-13 14:02 ` Willem de Bruijn
2023-09-13 18:04 ` Jordan Rife
2023-09-13 19:10 ` Willem de Bruijn
2023-09-14 8:24 ` Paolo Abeni
2023-09-14 18:41 ` Jordan Rife
2023-09-15 0:16 ` Willem de Bruijn
2023-09-15 0:21 ` Jordan Rife
2023-09-13 7:19 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=65006891779ed_25e754294b8@willemb.c.googlers.com.notmuch \
--to=willemdebruijn.kernel@gmail.com \
--cc=davem@davemloft.net \
--cc=dborkman@kernel.org \
--cc=edumazet@google.com \
--cc=jrife@google.com \
--cc=kuba@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).