diff mbox series

cifs: fix mount on old smb servers

Message ID 20230216183322@manguebit.com (mailing list archive)
State New, archived
Headers show
Series cifs: fix mount on old smb servers | expand

Commit Message

Paulo Alcantara Feb. 16, 2023, 6:33 p.m. UTC
The client was sending rfc1002 session request packet with a wrong
length field set, therefore failing to mount shares against old SMB
servers over port 139.

Fix this by calculating the correct length as specified in rfc1002.

Fixes: d7173623bf0b ("cifs: use ALIGN() and round_up() macros")
Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
---
 fs/cifs/connect.c | 100 ++++++++++++++++++----------------------------
 1 file changed, 38 insertions(+), 62 deletions(-)

Comments

ronnie sahlberg Feb. 16, 2023, 9:08 p.m. UTC | #1
Very nice cleanup.

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>

On Fri, 17 Feb 2023 at 04:44, Paulo Alcantara <pc@manguebit.com> wrote:
>
> The client was sending rfc1002 session request packet with a wrong
> length field set, therefore failing to mount shares against old SMB
> servers over port 139.
>
> Fix this by calculating the correct length as specified in rfc1002.
>
> Fixes: d7173623bf0b ("cifs: use ALIGN() and round_up() macros")
> Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> ---
>  fs/cifs/connect.c | 100 ++++++++++++++++++----------------------------
>  1 file changed, 38 insertions(+), 62 deletions(-)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index b2a04b4e89a5..af49ae53aaf4 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2843,72 +2843,48 @@ ip_rfc1001_connect(struct TCP_Server_Info *server)
>          * negprot - BB check reconnection in case where second
>          * sessinit is sent but no second negprot
>          */
> -       struct rfc1002_session_packet *ses_init_buf;
> -       unsigned int req_noscope_len;
> -       struct smb_hdr *smb_buf;
> +       struct rfc1002_session_packet req = {};
> +       struct smb_hdr *smb_buf = (struct smb_hdr *)&req;
> +       unsigned int len;
> +
> +       req.trailer.session_req.called_len = sizeof(req.trailer.session_req.called_name);
> +
> +       if (server->server_RFC1001_name[0] != 0)
> +               rfc1002mangle(req.trailer.session_req.called_name,
> +                             server->server_RFC1001_name,
> +                             RFC1001_NAME_LEN_WITH_NULL);
> +       else
> +               rfc1002mangle(req.trailer.session_req.called_name,
> +                             DEFAULT_CIFS_CALLED_NAME,
> +                             RFC1001_NAME_LEN_WITH_NULL);
> +
> +       req.trailer.session_req.calling_len = sizeof(req.trailer.session_req.calling_name);
> +
> +       /* calling name ends in null (byte 16) from old smb convention */
> +       if (server->workstation_RFC1001_name[0] != 0)
> +               rfc1002mangle(req.trailer.session_req.calling_name,
> +                             server->workstation_RFC1001_name,
> +                             RFC1001_NAME_LEN_WITH_NULL);
> +       else
> +               rfc1002mangle(req.trailer.session_req.calling_name,
> +                             "LINUX_CIFS_CLNT",
> +                             RFC1001_NAME_LEN_WITH_NULL);
>
> -       ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> -                              GFP_KERNEL);
> -
> -       if (ses_init_buf) {
> -               ses_init_buf->trailer.session_req.called_len = 32;
> -
> -               if (server->server_RFC1001_name[0] != 0)
> -                       rfc1002mangle(ses_init_buf->trailer.
> -                                     session_req.called_name,
> -                                     server->server_RFC1001_name,
> -                                     RFC1001_NAME_LEN_WITH_NULL);
> -               else
> -                       rfc1002mangle(ses_init_buf->trailer.
> -                                     session_req.called_name,
> -                                     DEFAULT_CIFS_CALLED_NAME,
> -                                     RFC1001_NAME_LEN_WITH_NULL);
> -
> -               ses_init_buf->trailer.session_req.calling_len = 32;
> -
> -               /*
> -                * calling name ends in null (byte 16) from old smb
> -                * convention.
> -                */
> -               if (server->workstation_RFC1001_name[0] != 0)
> -                       rfc1002mangle(ses_init_buf->trailer.
> -                                     session_req.calling_name,
> -                                     server->workstation_RFC1001_name,
> -                                     RFC1001_NAME_LEN_WITH_NULL);
> -               else
> -                       rfc1002mangle(ses_init_buf->trailer.
> -                                     session_req.calling_name,
> -                                     "LINUX_CIFS_CLNT",
> -                                     RFC1001_NAME_LEN_WITH_NULL);
> -
> -               ses_init_buf->trailer.session_req.scope1 = 0;
> -               ses_init_buf->trailer.session_req.scope2 = 0;
> -               smb_buf = (struct smb_hdr *)ses_init_buf;
> -
> -               /* sizeof RFC1002_SESSION_REQUEST with no scopes */
> -               req_noscope_len = sizeof(struct rfc1002_session_packet) - 2;
> +       /*
> +        * As per rfc1002, @len must be the number of bytes that follows the
> +        * length field of a rfc1002 session request payload.
> +        */
> +       len = sizeof(req) - offsetof(struct rfc1002_session_packet, trailer.session_req);
>
> -               /* == cpu_to_be32(0x81000044) */
> -               smb_buf->smb_buf_length =
> -                       cpu_to_be32((RFC1002_SESSION_REQUEST << 24) | req_noscope_len);
> -               rc = smb_send(server, smb_buf, 0x44);
> -               kfree(ses_init_buf);
> -               /*
> -                * RFC1001 layer in at least one server
> -                * requires very short break before negprot
> -                * presumably because not expecting negprot
> -                * to follow so fast.  This is a simple
> -                * solution that works without
> -                * complicating the code and causes no
> -                * significant slowing down on mount
> -                * for everyone else
> -                */
> -               usleep_range(1000, 2000);
> -       }
> +       smb_buf->smb_buf_length = cpu_to_be32((RFC1002_SESSION_REQUEST << 24) | len);
> +       rc = smb_send(server, smb_buf, len);
>         /*
> -        * else the negprot may still work without this
> -        * even though malloc failed
> +        * RFC1001 layer in at least one server requires very short break before
> +        * negprot presumably because not expecting negprot to follow so fast.
> +        * This is a simple solution that works without complicating the code
> +        * and causes no significant slowing down on mount for everyone else
>          */
> +       usleep_range(1000, 2000);
>
>         return rc;
>  }
> --
> 2.39.1
>
Steve French Feb. 17, 2023, 5:33 a.m. UTC | #2
merged into cifs-2.6.git for-next

On Thu, Feb 16, 2023 at 3:08 PM ronnie sahlberg
<ronniesahlberg@gmail.com> wrote:
>
> Very nice cleanup.
>
> Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>
>
> On Fri, 17 Feb 2023 at 04:44, Paulo Alcantara <pc@manguebit.com> wrote:
> >
> > The client was sending rfc1002 session request packet with a wrong
> > length field set, therefore failing to mount shares against old SMB
> > servers over port 139.
> >
> > Fix this by calculating the correct length as specified in rfc1002.
> >
> > Fixes: d7173623bf0b ("cifs: use ALIGN() and round_up() macros")
> > Signed-off-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
> > ---
> >  fs/cifs/connect.c | 100 ++++++++++++++++++----------------------------
> >  1 file changed, 38 insertions(+), 62 deletions(-)
> >
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index b2a04b4e89a5..af49ae53aaf4 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -2843,72 +2843,48 @@ ip_rfc1001_connect(struct TCP_Server_Info *server)
> >          * negprot - BB check reconnection in case where second
> >          * sessinit is sent but no second negprot
> >          */
> > -       struct rfc1002_session_packet *ses_init_buf;
> > -       unsigned int req_noscope_len;
> > -       struct smb_hdr *smb_buf;
> > +       struct rfc1002_session_packet req = {};
> > +       struct smb_hdr *smb_buf = (struct smb_hdr *)&req;
> > +       unsigned int len;
> > +
> > +       req.trailer.session_req.called_len = sizeof(req.trailer.session_req.called_name);
> > +
> > +       if (server->server_RFC1001_name[0] != 0)
> > +               rfc1002mangle(req.trailer.session_req.called_name,
> > +                             server->server_RFC1001_name,
> > +                             RFC1001_NAME_LEN_WITH_NULL);
> > +       else
> > +               rfc1002mangle(req.trailer.session_req.called_name,
> > +                             DEFAULT_CIFS_CALLED_NAME,
> > +                             RFC1001_NAME_LEN_WITH_NULL);
> > +
> > +       req.trailer.session_req.calling_len = sizeof(req.trailer.session_req.calling_name);
> > +
> > +       /* calling name ends in null (byte 16) from old smb convention */
> > +       if (server->workstation_RFC1001_name[0] != 0)
> > +               rfc1002mangle(req.trailer.session_req.calling_name,
> > +                             server->workstation_RFC1001_name,
> > +                             RFC1001_NAME_LEN_WITH_NULL);
> > +       else
> > +               rfc1002mangle(req.trailer.session_req.calling_name,
> > +                             "LINUX_CIFS_CLNT",
> > +                             RFC1001_NAME_LEN_WITH_NULL);
> >
> > -       ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
> > -                              GFP_KERNEL);
> > -
> > -       if (ses_init_buf) {
> > -               ses_init_buf->trailer.session_req.called_len = 32;
> > -
> > -               if (server->server_RFC1001_name[0] != 0)
> > -                       rfc1002mangle(ses_init_buf->trailer.
> > -                                     session_req.called_name,
> > -                                     server->server_RFC1001_name,
> > -                                     RFC1001_NAME_LEN_WITH_NULL);
> > -               else
> > -                       rfc1002mangle(ses_init_buf->trailer.
> > -                                     session_req.called_name,
> > -                                     DEFAULT_CIFS_CALLED_NAME,
> > -                                     RFC1001_NAME_LEN_WITH_NULL);
> > -
> > -               ses_init_buf->trailer.session_req.calling_len = 32;
> > -
> > -               /*
> > -                * calling name ends in null (byte 16) from old smb
> > -                * convention.
> > -                */
> > -               if (server->workstation_RFC1001_name[0] != 0)
> > -                       rfc1002mangle(ses_init_buf->trailer.
> > -                                     session_req.calling_name,
> > -                                     server->workstation_RFC1001_name,
> > -                                     RFC1001_NAME_LEN_WITH_NULL);
> > -               else
> > -                       rfc1002mangle(ses_init_buf->trailer.
> > -                                     session_req.calling_name,
> > -                                     "LINUX_CIFS_CLNT",
> > -                                     RFC1001_NAME_LEN_WITH_NULL);
> > -
> > -               ses_init_buf->trailer.session_req.scope1 = 0;
> > -               ses_init_buf->trailer.session_req.scope2 = 0;
> > -               smb_buf = (struct smb_hdr *)ses_init_buf;
> > -
> > -               /* sizeof RFC1002_SESSION_REQUEST with no scopes */
> > -               req_noscope_len = sizeof(struct rfc1002_session_packet) - 2;
> > +       /*
> > +        * As per rfc1002, @len must be the number of bytes that follows the
> > +        * length field of a rfc1002 session request payload.
> > +        */
> > +       len = sizeof(req) - offsetof(struct rfc1002_session_packet, trailer.session_req);
> >
> > -               /* == cpu_to_be32(0x81000044) */
> > -               smb_buf->smb_buf_length =
> > -                       cpu_to_be32((RFC1002_SESSION_REQUEST << 24) | req_noscope_len);
> > -               rc = smb_send(server, smb_buf, 0x44);
> > -               kfree(ses_init_buf);
> > -               /*
> > -                * RFC1001 layer in at least one server
> > -                * requires very short break before negprot
> > -                * presumably because not expecting negprot
> > -                * to follow so fast.  This is a simple
> > -                * solution that works without
> > -                * complicating the code and causes no
> > -                * significant slowing down on mount
> > -                * for everyone else
> > -                */
> > -               usleep_range(1000, 2000);
> > -       }
> > +       smb_buf->smb_buf_length = cpu_to_be32((RFC1002_SESSION_REQUEST << 24) | len);
> > +       rc = smb_send(server, smb_buf, len);
> >         /*
> > -        * else the negprot may still work without this
> > -        * even though malloc failed
> > +        * RFC1001 layer in at least one server requires very short break before
> > +        * negprot presumably because not expecting negprot to follow so fast.
> > +        * This is a simple solution that works without complicating the code
> > +        * and causes no significant slowing down on mount for everyone else
> >          */
> > +       usleep_range(1000, 2000);
> >
> >         return rc;
> >  }
> > --
> > 2.39.1
> >
diff mbox series

Patch

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index b2a04b4e89a5..af49ae53aaf4 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2843,72 +2843,48 @@  ip_rfc1001_connect(struct TCP_Server_Info *server)
 	 * negprot - BB check reconnection in case where second
 	 * sessinit is sent but no second negprot
 	 */
-	struct rfc1002_session_packet *ses_init_buf;
-	unsigned int req_noscope_len;
-	struct smb_hdr *smb_buf;
+	struct rfc1002_session_packet req = {};
+	struct smb_hdr *smb_buf = (struct smb_hdr *)&req;
+	unsigned int len;
+
+	req.trailer.session_req.called_len = sizeof(req.trailer.session_req.called_name);
+
+	if (server->server_RFC1001_name[0] != 0)
+		rfc1002mangle(req.trailer.session_req.called_name,
+			      server->server_RFC1001_name,
+			      RFC1001_NAME_LEN_WITH_NULL);
+	else
+		rfc1002mangle(req.trailer.session_req.called_name,
+			      DEFAULT_CIFS_CALLED_NAME,
+			      RFC1001_NAME_LEN_WITH_NULL);
+
+	req.trailer.session_req.calling_len = sizeof(req.trailer.session_req.calling_name);
+
+	/* calling name ends in null (byte 16) from old smb convention */
+	if (server->workstation_RFC1001_name[0] != 0)
+		rfc1002mangle(req.trailer.session_req.calling_name,
+			      server->workstation_RFC1001_name,
+			      RFC1001_NAME_LEN_WITH_NULL);
+	else
+		rfc1002mangle(req.trailer.session_req.calling_name,
+			      "LINUX_CIFS_CLNT",
+			      RFC1001_NAME_LEN_WITH_NULL);
 
-	ses_init_buf = kzalloc(sizeof(struct rfc1002_session_packet),
-			       GFP_KERNEL);
-
-	if (ses_init_buf) {
-		ses_init_buf->trailer.session_req.called_len = 32;
-
-		if (server->server_RFC1001_name[0] != 0)
-			rfc1002mangle(ses_init_buf->trailer.
-				      session_req.called_name,
-				      server->server_RFC1001_name,
-				      RFC1001_NAME_LEN_WITH_NULL);
-		else
-			rfc1002mangle(ses_init_buf->trailer.
-				      session_req.called_name,
-				      DEFAULT_CIFS_CALLED_NAME,
-				      RFC1001_NAME_LEN_WITH_NULL);
-
-		ses_init_buf->trailer.session_req.calling_len = 32;
-
-		/*
-		 * calling name ends in null (byte 16) from old smb
-		 * convention.
-		 */
-		if (server->workstation_RFC1001_name[0] != 0)
-			rfc1002mangle(ses_init_buf->trailer.
-				      session_req.calling_name,
-				      server->workstation_RFC1001_name,
-				      RFC1001_NAME_LEN_WITH_NULL);
-		else
-			rfc1002mangle(ses_init_buf->trailer.
-				      session_req.calling_name,
-				      "LINUX_CIFS_CLNT",
-				      RFC1001_NAME_LEN_WITH_NULL);
-
-		ses_init_buf->trailer.session_req.scope1 = 0;
-		ses_init_buf->trailer.session_req.scope2 = 0;
-		smb_buf = (struct smb_hdr *)ses_init_buf;
-
-		/* sizeof RFC1002_SESSION_REQUEST with no scopes */
-		req_noscope_len = sizeof(struct rfc1002_session_packet) - 2;
+	/*
+	 * As per rfc1002, @len must be the number of bytes that follows the
+	 * length field of a rfc1002 session request payload.
+	 */
+	len = sizeof(req) - offsetof(struct rfc1002_session_packet, trailer.session_req);
 
-		/* == cpu_to_be32(0x81000044) */
-		smb_buf->smb_buf_length =
-			cpu_to_be32((RFC1002_SESSION_REQUEST << 24) | req_noscope_len);
-		rc = smb_send(server, smb_buf, 0x44);
-		kfree(ses_init_buf);
-		/*
-		 * RFC1001 layer in at least one server
-		 * requires very short break before negprot
-		 * presumably because not expecting negprot
-		 * to follow so fast.  This is a simple
-		 * solution that works without
-		 * complicating the code and causes no
-		 * significant slowing down on mount
-		 * for everyone else
-		 */
-		usleep_range(1000, 2000);
-	}
+	smb_buf->smb_buf_length = cpu_to_be32((RFC1002_SESSION_REQUEST << 24) | len);
+	rc = smb_send(server, smb_buf, len);
 	/*
-	 * else the negprot may still work without this
-	 * even though malloc failed
+	 * RFC1001 layer in at least one server requires very short break before
+	 * negprot presumably because not expecting negprot to follow so fast.
+	 * This is a simple solution that works without complicating the code
+	 * and causes no significant slowing down on mount for everyone else
 	 */
+	usleep_range(1000, 2000);
 
 	return rc;
 }