| Message ID | 20230216183322@manguebit.com |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: fix mount on old smb servers | expand |
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 >
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 --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; }
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(-)