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
>
