| Message ID | 20231004011303.979995-1-jrife@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | smb: use kernel_connect() and kernel_bind() | expand |
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>
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>
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
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
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(-)