Re: [PATCH net] net: prevent address overwrite in connect() and sendmsg()

[Jordan Rife <jrife@google.com>]

> These should probably call kernel_connect instead.
> Similarly, this could call kernel_sendmsg, and the extra copy handled in that wrapper.

I was considering this approach initially, but one concern I had was
that there are other instances I see of modules calling ops->connect()
directly (bypassing kernel_connect()):

- net/netfilter/ipvs/ip_vs_sync.c: make_send_sock()
- drivers/block/drbd/drbd_receiver.c: drbd_try_connect()
- drivers/infiniband/hw/erdma/erdma_cm.c: kernel_bindconnect()
- drivers/infiniband/sw/siw/siw_cm.c: kernel_bindconnect()
- fs/ocfs2/cluster/tcp.c: o2net_start_connect()
- net/rds/tcp_connect.c: rds_tcp_conn_path_connect()

and ops->sendmsg():

- net/smc/af_smc.c: smc_sendmsg()
- drivers/vhost/net.c: vhost_tx_batch(), handle_tx_copy(), handle_tx_zerocopy()
- drivers/target/iscsi/iscsi_target_util.c: iscsit_fe_sendpage_sg()

which (at least in theory) leaves them open to the same problem I'm
seeing with NFS/SMB right now. I worry that even if all these
instances were swapped out with kernel_sendmsg() and kernel_connect(),
it would turn into a game of whac-a-mole in the future as new changes
or new modules may reintroduce direct calls to sock->ops->connect() or
sock->ops->sendmsg().

-Jordan



On Tue, Sep 12, 2023 at 7:22 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/12/23 3:33 PM, Willem de Bruijn wrote:
> > 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.
>
> Fully agree, if it's feasible it would be better to convert them over to
> in-kernel API.
>
> >> 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>