| Message ID | 20230918025021.4078252-2-jrife@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2,1/3] net: replace calls to sock->ops->connect() with kernel_connect() | expand |
On Sun, Sep 17, 2023 at 10:50 PM Jordan Rife <jrife@google.com> 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. You used this short-hand to avoid having to update all callers to sock_sendmsg to __kernel_sendmsg? Unless the changes are massively worse than the other two patches, I do think using the kernel_.. prefix is preferable, as it documents that in-kernel users should use the kernel_.. sockets API rather than directly call the sock_.. ones. It's not clear that sock_sendmsg really is part of the kernel socket API.
> You used this short-hand to avoid having to update all callers to sock_sendmsg to __kernel_sendmsg? Sorry about that, I misunderstood the intent. I'm fine with introducing a new function, doing the address copy there, and replacing all calls to sock_sendmsg with this wrapper. One thought on the naming though, To me the "__" prefix seems out of place for a function meant as a public interface. Some possible alternatives: 1) Rename the current kernel_sendmsg() function to kernel_sendmsg_kvec() and name our new function kernel_sendmsg(). To me this makes some sense, considering the new function is the more generic of the two, and the existing kernel_sendmsg() specifically accepts "struct kvec". 2) Same as #1, but drop the old kernel_sendmsg() function instead of renaming it. Adapt all calls to the old kernel_sendmsg() to fit the new kernel_sendmsg() (this amounts to adding a iov_iter_kvec(&msg->msg_iter, ITER_SOURCE, vec, num, size); call in each spot before kernel_sendmsg() is called. -Jordan On Mon, Sep 18, 2023 at 6:09 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Sun, Sep 17, 2023 at 10:50 PM Jordan Rife <jrife@google.com> 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. > > You used this short-hand to avoid having to update all callers to > sock_sendmsg to __kernel_sendmsg? > > Unless the changes are massively worse than the other two patches, I > do think using the kernel_.. prefix is preferable, as it documents > that in-kernel users should use the kernel_.. sockets API rather than > directly call the sock_.. ones. > > It's not clear that sock_sendmsg really is part of the kernel socket API.
On Mon, Sep 18, 2023 at 1:52 PM Jordan Rife <jrife@google.com> wrote: > > > You used this short-hand to avoid having to update all callers to sock_sendmsg to __kernel_sendmsg? > > Sorry about that, I misunderstood the intent. I'm fine with > introducing a new function, doing the address copy there, and > replacing all calls to sock_sendmsg with this wrapper. One thought on > the naming though, > > To me the "__" prefix seems out of place for a function meant as a > public interface. Some possible alternatives: > > 1) Rename the current kernel_sendmsg() function to > kernel_sendmsg_kvec() and name our new function kernel_sendmsg(). To > me this makes some sense, considering the new function is the more > generic of the two, and the existing kernel_sendmsg() specifically > accepts "struct kvec". > 2) Same as #1, but drop the old kernel_sendmsg() function instead of > renaming it. Adapt all calls to the old kernel_sendmsg() to fit the > new kernel_sendmsg() (this amounts to adding a > iov_iter_kvec(&msg->msg_iter, ITER_SOURCE, vec, num, size); call in > each spot before kernel_sendmsg() is called. Thanks. Fair points. Of the two proposals, I think the first is preferable, as it avoids some non-trivial open coding in multiple callers.
Sounds like a plan. On Mon, Sep 18, 2023 at 10:55 AM Willem de Bruijn <willemdebruijn.kernel@gmail.com> wrote: > > On Mon, Sep 18, 2023 at 1:52 PM Jordan Rife <jrife@google.com> wrote: > > > > > You used this short-hand to avoid having to update all callers to sock_sendmsg to __kernel_sendmsg? > > > > Sorry about that, I misunderstood the intent. I'm fine with > > introducing a new function, doing the address copy there, and > > replacing all calls to sock_sendmsg with this wrapper. One thought on > > the naming though, > > > > To me the "__" prefix seems out of place for a function meant as a > > public interface. Some possible alternatives: > > > > 1) Rename the current kernel_sendmsg() function to > > kernel_sendmsg_kvec() and name our new function kernel_sendmsg(). To > > me this makes some sense, considering the new function is the more > > generic of the two, and the existing kernel_sendmsg() specifically > > accepts "struct kvec". > > 2) Same as #1, but drop the old kernel_sendmsg() function instead of > > renaming it. Adapt all calls to the old kernel_sendmsg() to fit the > > new kernel_sendmsg() (this amounts to adding a > > iov_iter_kvec(&msg->msg_iter, ITER_SOURCE, vec, num, size); call in > > each spot before kernel_sendmsg() is called. > > Thanks. Fair points. > > Of the two proposals, I think the first is preferable, as it avoids > some non-trivial open coding in multiple callers.
Just a heads up, there are also kernel_recvmsg/sock_recvmsg functions that mirror the kernel_sendmsg/sock_sendmsg. If we are are doing this > 1) Rename the current kernel_sendmsg() function to > kernel_sendmsg_kvec() and name our new function kernel_sendmsg(). To > me this makes some sense, considering the new function is the more > generic of the two, and the existing kernel_sendmsg() specifically > accepts "struct kvec". it creates an asymmetry between *sendmsg and *recvmsg function names. If we wanted these to be similar then it means a rename to these functions (e.g. kern_recvmsg becomes kern_recvmsg_kvec, rename sock_recvmsg to kern_recvmsg). -Jordan On Mon, Sep 18, 2023 at 11:02 AM Jordan Rife <jrife@google.com> wrote: > > Sounds like a plan. > > On Mon, Sep 18, 2023 at 10:55 AM Willem de Bruijn > <willemdebruijn.kernel@gmail.com> wrote: > > > > On Mon, Sep 18, 2023 at 1:52 PM Jordan Rife <jrife@google.com> wrote: > > > > > > > You used this short-hand to avoid having to update all callers to sock_sendmsg to __kernel_sendmsg? > > > > > > Sorry about that, I misunderstood the intent. I'm fine with > > > introducing a new function, doing the address copy there, and > > > replacing all calls to sock_sendmsg with this wrapper. One thought on > > > the naming though, > > > > > > To me the "__" prefix seems out of place for a function meant as a > > > public interface. Some possible alternatives: > > > > > > 1) Rename the current kernel_sendmsg() function to > > > kernel_sendmsg_kvec() and name our new function kernel_sendmsg(). To > > > me this makes some sense, considering the new function is the more > > > generic of the two, and the existing kernel_sendmsg() specifically > > > accepts "struct kvec". > > > 2) Same as #1, but drop the old kernel_sendmsg() function instead of > > > renaming it. Adapt all calls to the old kernel_sendmsg() to fit the > > > new kernel_sendmsg() (this amounts to adding a > > > iov_iter_kvec(&msg->msg_iter, ITER_SOURCE, vec, num, size); call in > > > each spot before kernel_sendmsg() is called. > > > > Thanks. Fair points. > > > > Of the two proposals, I think the first is preferable, as it avoids > > some non-trivial open coding in multiple callers.
On Mon, Sep 18, 2023 at 4:49 PM Jordan Rife <jrife@google.com> wrote: > > Just a heads up, there are also kernel_recvmsg/sock_recvmsg functions > that mirror the kernel_sendmsg/sock_sendmsg. If we are are doing this > > > 1) Rename the current kernel_sendmsg() function to > > kernel_sendmsg_kvec() and name our new function kernel_sendmsg(). To > > me this makes some sense, considering the new function is the more > > generic of the two, and the existing kernel_sendmsg() specifically > > accepts "struct kvec". > > it creates an asymmetry between *sendmsg and *recvmsg function names. > If we wanted these to be similar then it means a rename to these > functions (e.g. kern_recvmsg becomes kern_recvmsg_kvec, rename > sock_recvmsg to kern_recvmsg). I see. That's definitely outside the realm of bug fix. If we have to keep the two consistent, then I suppose your existing fix is the best approach for net. And any renaming to clarify the API is best left for net-next.
diff --git a/net/socket.c b/net/socket.c index b2e3700d035a6..b0189b773d130 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,22 @@ 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 address; + struct sockaddr_storage *save_addr = (struct sockaddr_storage *)msg->msg_name; + int ret; - return err ?: sock_sendmsg_nosec(sock, msg); + if (msg->msg_name) { + if (msg->msg_namelen < 0 || msg->msg_namelen > sizeof(address)) + return -EINVAL; + + 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 +1158,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 +2194,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 +2558,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.
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/ Signed-off-by: Jordan Rife <jrife@google.com> --- v1->v2: Split up original patch into patch series. Perform address copy in sock_sendmsg() instead of sock->ops->sendmsg(). net/socket.c | 32 ++++++++++++++++++++++++++------ 1 file changed, 26 insertions(+), 6 deletions(-)