merged into cifs-2.6.git for-next
On Thu, Feb 16, 2023 at 3:08 PM ronnie sahlberg
<ronniesahlberg@xxxxxxxxx> wrote:
>
> Very nice cleanup.
>
> Reviewed-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx>
>
> On Fri, 17 Feb 2023 at 04:44, Paulo Alcantara <pc@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> > 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
> >
--
Thanks,
Steve
