Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH][SMB3] remove confusing dmesg when mounting with encryption
@ 2019-11-05 22:47 Steve French
  2019-11-08  7:07 ` Steve French
  0 siblings, 1 reply; 2+ messages in thread
From: Steve French @ 2019-11-05 22:47 UTC (permalink / raw)
  To: CIFS

[-- Attachment #1: Type: text/plain, Size: 525 bytes --]

The smb2/smb3 message checking code was logging to dmesg when mounting
with encryption ("seal") for compounded SMB3 requests.  When encrypted
the whole frame (including potentially multiple compounds) is read
so the length field is longer than in the case of non-encrypted
case (where length field will match the the calculated length for
the particular SMB3 request in the compound being validated).

Avoids the warning on mount (with "seal"):

   "srv rsp padded more than expected. Length 384 not ..."

-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-remove-confusing-dmesg-when-mounting-with-encry.patch --]
[-- Type: text/x-patch, Size: 5756 bytes --]

From 429f3ce8446a9af2cf306a0894ef7a3cb512d7fe Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 5 Nov 2019 16:36:41 -0600
Subject: [PATCH] smb3: remove confusing dmesg when mounting with encryption
 ("seal")

The smb2/smb3 message checking code was logging to dmesg when mounting
with encryption ("seal") for compounded SMB3 requests.  When encrypted
the whole frame (including potentially multiple compounds) is read
so the length field is longer than in the case of non-encrypted
case (where length field will match the the calculated length for
the particular SMB3 request in the compound being validated).

Avoids the warning on mount (with "seal"):

   "srv rsp padded more than expected. Length 384 not ..."

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifsglob.h  |  3 ++-
 fs/cifs/cifsproto.h |  3 ++-
 fs/cifs/connect.c   |  3 ++-
 fs/cifs/misc.c      |  3 ++-
 fs/cifs/smb2misc.c  | 18 ++++++++++++++----
 fs/cifs/smb2proto.h |  2 +-
 6 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 77a807408877..aa0fca853d4b 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -266,7 +266,8 @@ struct smb_version_operations {
 	void (*print_stats)(struct seq_file *m, struct cifs_tcon *);
 	void (*dump_share_caps)(struct seq_file *, struct cifs_tcon *);
 	/* verify the message */
-	int (*check_message)(char *, unsigned int, struct TCP_Server_Info *);
+	int (*check_message)(char *buf, unsigned int len,
+			     struct TCP_Server_Info *srv, bool decrypted);
 	bool (*is_oplock_break)(char *, struct TCP_Server_Info *);
 	int (*handle_cancelled_mid)(char *, struct TCP_Server_Info *);
 	void (*downgrade_oplock)(struct TCP_Server_Info *,
diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 1ed695336f62..f9c4a821c3da 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -128,7 +128,8 @@ extern int SendReceiveBlockingLock(const unsigned int xid,
 			struct smb_hdr *out_buf,
 			int *bytes_returned);
 extern int cifs_reconnect(struct TCP_Server_Info *server);
-extern int checkSMB(char *buf, unsigned int len, struct TCP_Server_Info *srvr);
+extern int checkSMB(char *buf, unsigned int len, struct TCP_Server_Info *srvr,
+		    bool decrypted);
 extern bool is_valid_oplock_break(char *, struct TCP_Server_Info *);
 extern bool backup_cred(struct cifs_sb_info *);
 extern bool is_size_safe_to_change(struct cifsInodeInfo *, __u64 eof);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index d1b6e9475fb7..dcc52c8ebd9e 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1071,7 +1071,8 @@ cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid)
 	 * 48 bytes is enough to display the header and a little bit
 	 * into the payload for debugging purposes.
 	 */
-	length = server->ops->check_message(buf, server->total_read, server);
+	length = server->ops->check_message(buf, server->total_read, server,
+					    mid->decrypted);
 	if (length != 0)
 		cifs_dump_mem("Bad SMB: ", buf,
 			min_t(unsigned int, server->total_read, 48));
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 5ad83bdb9bea..a14759d14a89 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -312,7 +312,8 @@ check_smb_hdr(struct smb_hdr *smb)
 }
 
 int
-checkSMB(char *buf, unsigned int total_read, struct TCP_Server_Info *server)
+checkSMB(char *buf, unsigned int total_read, struct TCP_Server_Info *server,
+	 bool decrypted /* unused for SMB1 */)
 {
 	struct smb_hdr *smb = (struct smb_hdr *)buf;
 	__u32 rfclen = be32_to_cpu(smb->smb_buf_length);
diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 4b7ff3d1e830..68c9fdbf3ff7 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -129,7 +129,8 @@ static __u32 get_neg_ctxt_len(struct smb2_sync_hdr *hdr, __u32 len,
 }
 
 int
-smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
+smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr,
+		   bool decrypted)
 {
 	struct smb2_sync_hdr *shdr = (struct smb2_sync_hdr *)buf;
 	struct smb2_sync_pdu *pdu = (struct smb2_sync_pdu *)shdr;
@@ -253,11 +254,20 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
 		 * If pad is longer than eight bytes, log the server behavior
 		 * (once), since may indicate a problem but allow it and continue
 		 * since the frame is parseable.
+		 * (once), since may indicate a problem but allow it and
+		 * continue since the frame is parseable.
+		 *
+		 * Do not log warning below if decrypted since for the encrypted
+		 * case len can include total of more than one SMB request
+		 * when part of a compounded req while clc_len will be smaller
+		 * since it is calculated for only one of the requests
 		 */
 		if (clc_len < len) {
-			pr_warn_once(
-			     "srv rsp padded more than expected. Length %d not %d for cmd:%d mid:%llu\n",
-			     len, clc_len, command, mid);
+			if (!decrypted)
+				pr_warn_once(
+				    "srv rsp padded more than expected. "
+				    "Length %d not %d for cmd:%d mid:%llu\n",
+				    len, clc_len, command, mid);
 			return 0;
 		}
 		pr_warn_once(
diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h
index a469fa211f37..50fd8a67accc 100644
--- a/fs/cifs/smb2proto.h
+++ b/fs/cifs/smb2proto.h
@@ -35,7 +35,7 @@ struct smb_rqst;
  */
 extern int map_smb2_to_linux_error(char *buf, bool log_err);
 extern int smb2_check_message(char *buf, unsigned int length,
-			      struct TCP_Server_Info *server);
+			      struct TCP_Server_Info *server, bool decrypted);
 extern unsigned int smb2_calc_size(void *buf, struct TCP_Server_Info *server);
 extern char *smb2_get_data_area_len(int *off, int *len,
 				    struct smb2_sync_hdr *shdr);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH][SMB3] remove confusing dmesg when mounting with encryption
  2019-11-05 22:47 [PATCH][SMB3] remove confusing dmesg when mounting with encryption Steve French
@ 2019-11-08  7:07 ` Steve French
  0 siblings, 0 replies; 2+ messages in thread
From: Steve French @ 2019-11-08  7:07 UTC (permalink / raw)
  To: CIFS, Dan Carpenter, Pavel Shilovsky

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

Here is updated patch which following Pavel's suggestion simply
removes the sometimes confusing warning rather than make a more
complex change which checks for whether the packet was decrypted
earlier or not


On Tue, Nov 5, 2019 at 4:47 PM Steve French <smfrench@gmail.com> wrote:
>
> The smb2/smb3 message checking code was logging to dmesg when mounting
> with encryption ("seal") for compounded SMB3 requests.  When encrypted
> the whole frame (including potentially multiple compounds) is read
> so the length field is longer than in the case of non-encrypted
> case (where length field will match the the calculated length for
> the particular SMB3 request in the compound being validated).
>
> Avoids the warning on mount (with "seal"):
>
>    "srv rsp padded more than expected. Length 384 not ..."
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-remove-confusing-dmesg-when-mounting-with-encry.patch --]
[-- Type: text/x-patch, Size: 1835 bytes --]

From 0ad92ed9bf9cd9e4bc9412d3f6e9ccbfcc27fb43 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 8 Nov 2019 01:01:35 -0600
Subject: [PATCH 1/2] smb3: remove confusing dmesg when mounting with
 encryption ("seal")

The smb2/smb3 message checking code was logging to dmesg when mounting
with encryption ("seal") for compounded SMB3 requests.  When encrypted
the whole frame (including potentially multiple compounds) is read
so the length field is longer than in the case of non-encrypted
case (where length field will match the the calculated length for
the particular SMB3 request in the compound being validated).

Avoids the warning on mount (with "seal"):

   "srv rsp padded more than expected. Length 384 not ..."

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/smb2misc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 4b7ff3d1e830..a524fc1fad84 100644
--- a/fs/cifs/smb2misc.c
+++ b/fs/cifs/smb2misc.c
@@ -250,16 +250,10 @@ smb2_check_message(char *buf, unsigned int len, struct TCP_Server_Info *srvr)
 		 * of junk. Other servers match RFC1001 len to actual
 		 * SMB2/SMB3 frame length (header + smb2 response specific data)
 		 * Some windows servers also pad up to 8 bytes when compounding.
-		 * If pad is longer than eight bytes, log the server behavior
-		 * (once), since may indicate a problem but allow it and continue
-		 * since the frame is parseable.
 		 */
-		if (clc_len < len) {
-			pr_warn_once(
-			     "srv rsp padded more than expected. Length %d not %d for cmd:%d mid:%llu\n",
-			     len, clc_len, command, mid);
+		if (clc_len < len)
 			return 0;
-		}
+
 		pr_warn_once(
 			"srv rsp too short, len %d not %d. cmd:%d mid:%llu\n",
 			len, clc_len, command, mid);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 22:47 [PATCH][SMB3] remove confusing dmesg when mounting with encryption Steve French
2019-11-08  7:07 ` Steve French

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org
	public-inbox-index linux-cifs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git