[PATCH] smb: use kernel_connect() and kernel_bind()

[PATCH] smb: use kernel_connect() and kernel_bind()

[Jordan Rife]

Recent changes to kernel_connect() and kernel_bind() ensure that
callers are insulated from changes to the address parameter made by BPF
SOCK_ADDR hooks. This patch wraps direct calls to ops->connect() and
ops->bind() with kernel_connect() and kernel_bind() to ensure that SMB
mounts do not see their mount address overwritten in such cases.

Link: https://lore.kernel.org/netdev/9944248dba1bce861375fcce9de663934d933ba9.camel@redhat.com/
Cc: <stable@vger.kernel.org> # 6.x.y
Signed-off-by: Jordan Rife <jrife@google.com>
---
 fs/smb/client/connect.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c
index 3902e90dca6b0..ce11165446cfb 100644
--- a/fs/smb/client/connect.c
+++ b/fs/smb/client/connect.c
@@ -2895,9 +2895,9 @@ bind_socket(struct TCP_Server_Info *server)
 	if (server->srcaddr.ss_family != AF_UNSPEC) {
 		/* Bind to the specified local IP address */
 		struct socket *socket = server->ssocket;
-		rc = socket->ops->bind(socket,
-				       (struct sockaddr *) &server->srcaddr,
-				       sizeof(server->srcaddr));
+		rc = kernel_bind(socket,
+				 (struct sockaddr *) &server->srcaddr,
+				 sizeof(server->srcaddr));
 		if (rc < 0) {
 			struct sockaddr_in *saddr4;
 			struct sockaddr_in6 *saddr6;
@@ -3046,8 +3046,8 @@ generic_ip_connect(struct TCP_Server_Info *server)
 		 socket->sk->sk_sndbuf,
 		 socket->sk->sk_rcvbuf, socket->sk->sk_rcvtimeo);
 
-	rc = socket->ops->connect(socket, saddr, slen,
-				  server->noblockcnt ? O_NONBLOCK : 0);
+	rc = kernel_connect(socket, saddr, slen,
+			    server->noblockcnt ? O_NONBLOCK : 0);
 	/*
 	 * When mounting SMB root file systems, we do not want to block in
 	 * connect. Otherwise bail out and then let cifs_reconnect() perform
-- 
2.42.0.582.g8ccd20d70d-goog

Re: [PATCH] smb: use kernel_connect() and kernel_bind()

[Paulo Alcantara]

Jordan Rife <jrife@google.com> writes:

> Recent changes to kernel_connect() and kernel_bind() ensure that
> callers are insulated from changes to the address parameter made by BPF
> SOCK_ADDR hooks. This patch wraps direct calls to ops->connect() and
> ops->bind() with kernel_connect() and kernel_bind() to ensure that SMB
> mounts do not see their mount address overwritten in such cases.
>
> Link: https://lore.kernel.org/netdev/9944248dba1bce861375fcce9de663934d933ba9.camel@redhat.com/
> Cc: <stable@vger.kernel.org> # 6.x.y
> Signed-off-by: Jordan Rife <jrife@google.com>
> ---
>  fs/smb/client/connect.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com>

Re: [PATCH] smb: use kernel_connect() and kernel_bind()

[Steve French]

tentatively merged into cifs-2.6.git for-next pending testing and
additional review

On Wed, Oct 4, 2023 at 10:44 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Jordan Rife <jrife@google.com> writes:
>
> > Recent changes to kernel_connect() and kernel_bind() ensure that
> > callers are insulated from changes to the address parameter made by BPF
> > SOCK_ADDR hooks. This patch wraps direct calls to ops->connect() and
> > ops->bind() with kernel_connect() and kernel_bind() to ensure that SMB
> > mounts do not see their mount address overwritten in such cases.
> >
> > Link: https://lore.kernel.org/netdev/9944248dba1bce861375fcce9de663934d933ba9.camel@redhat.com/
> > Cc: <stable@vger.kernel.org> # 6.x.y
> > Signed-off-by: Jordan Rife <jrife@google.com>
> > ---
> >  fs/smb/client/connect.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
>
> Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com>



-- 
Thanks,

Steve

Re: [PATCH] smb: use kernel_connect() and kernel_bind()

[Jordan Rife]

On Wed, Oct 4, 2023 at 10:15 AM Steve French <smfrench@gmail.com> wrote:
>
> tentatively merged into cifs-2.6.git for-next pending testing and
> additional review
>
> On Wed, Oct 4, 2023 at 10:44 AM Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > Jordan Rife <jrife@google.com> writes:
> >
> > > Recent changes to kernel_connect() and kernel_bind() ensure that
> > > callers are insulated from changes to the address parameter made by BPF
> > > SOCK_ADDR hooks. This patch wraps direct calls to ops->connect() and
> > > ops->bind() with kernel_connect() and kernel_bind() to ensure that SMB
> > > mounts do not see their mount address overwritten in such cases.
> > >
> > > Link: https://lore.kernel.org/netdev/9944248dba1bce861375fcce9de663934d933ba9.camel@redhat.com/
> > > Cc: <stable@vger.kernel.org> # 6.x.y
> > > Signed-off-by: Jordan Rife <jrife@google.com>
> > > ---
> > >  fs/smb/client/connect.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > Acked-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
>
>
>
> --
> Thanks,
>
> Steve

> Do you want this to go through the cifs tree?

Yes. This was originally a part of a larger patch set destined for the
net tree (https://lore.kernel.org/netdev/20230919175159.144073-1-jrife@google.com/T/#u).
It was ultimately decided
(https://lore.kernel.org/netdev/20230919175323.144902-1-jrife@google.com/T/#m905ead9bdce794112a6cdc440f6887b787532023)
over there to try sending patches to the appropriate trees to avoid
merge conflicts.

> How urgent do you think it is (or should it wait for 6.7)?

Not super urgent, but ultimately it should be backported to stable
kernels 4.19+ (all versions where it's possible to overwrite the
address parameter with BPF hooks). The risk here is in environments
where BPF hooks are used to rewrite the connect/bind addresses (common
in systems like Kubernetes w/ Cilium). Doing so can break your mounts,
since the original mount address is "forgotten" when a call to
ops->connect() overwrites it (have confirmed this scenario myself).
IME, this scenario is more common to see with NFS mounts, but still
possible with SMB.

- Jordan