[PATCH 1/3] cifs: log session id when a matching ses is not found

[PATCH 1/3] cifs: log session id when a matching ses is not found

[Shyam Prasad N]

We do not log the session id in crypt_setup when a matching
session is not found. Printing the session id helps debugging
here. This change does just that.

Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/smb2ops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index a8bb9d00d33a..8e4900f6cd53 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -4439,8 +4439,8 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
 
 	rc = smb2_get_enc_key(server, le64_to_cpu(tr_hdr->SessionId), enc, key);
 	if (rc) {
-		cifs_server_dbg(VFS, "%s: Could not get %scryption key\n", __func__,
-			 enc ? "en" : "de");
+		cifs_server_dbg(VFS, "%s: Could not get %scryption key. sid: 0x%llx\n", __func__,
+			 enc ? "en" : "de", le64_to_cpu(tr_hdr->SessionId));
 		return rc;
 	}
 
-- 
2.34.1

[PATCH 2/3] cifs: prevent use-after-free by freeing the cfile later

[Shyam Prasad N]

In smb2_compound_op we have a possible use-after-free
which can cause hard to debug problems later on.

This was revealed during stress testing with KASAN enabled
kernel. Fixing it by moving the cfile free call to
a few lines below, after the usage.

Fixes: 76894f3e2f71 ("cifs: improve symlink handling for smb2+")
CC: Paulo Alcantara (SUSE) <pc@cjr.nz>
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/smb2inode.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/smb/client/smb2inode.c b/fs/smb/client/smb2inode.c
index 163a03298430..7e3ac4cb4efa 100644
--- a/fs/smb/client/smb2inode.c
+++ b/fs/smb/client/smb2inode.c
@@ -398,9 +398,6 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 					rsp_iov);
 
  finished:
-	if (cfile)
-		cifsFileInfo_put(cfile);
-
 	SMB2_open_free(&rqst[0]);
 	if (rc == -EREMCHG) {
 		pr_warn_once("server share %s deleted\n", tcon->tree_name);
@@ -529,6 +526,9 @@ static int smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon,
 		break;
 	}
 
+	if (cfile)
+		cifsFileInfo_put(cfile);
+
 	if (rc && err_iov && err_buftype) {
 		memcpy(err_iov, rsp_iov, 3 * sizeof(*err_iov));
 		memcpy(err_buftype, resp_buftype, 3 * sizeof(*err_buftype));
-- 
2.34.1

Re: [PATCH 2/3] cifs: prevent use-after-free by freeing the cfile later

[Paulo Alcantara]

Shyam Prasad N <nspmangalore@gmail.com> writes:

> In smb2_compound_op we have a possible use-after-free
> which can cause hard to debug problems later on.
>
> This was revealed during stress testing with KASAN enabled
> kernel. Fixing it by moving the cfile free call to
> a few lines below, after the usage.
>
> Fixes: 76894f3e2f71 ("cifs: improve symlink handling for smb2+")
> CC: Paulo Alcantara (SUSE) <pc@cjr.nz>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/smb2inode.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>

Re: [PATCH 2/3] cifs: prevent use-after-free by freeing the cfile later

[Steve French]

merged into cifs-2.6.git for-next

On Thu, Jun 22, 2023 at 1:46 PM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
>
> > In smb2_compound_op we have a possible use-after-free
> > which can cause hard to debug problems later on.
> >
> > This was revealed during stress testing with KASAN enabled
> > kernel. Fixing it by moving the cfile free call to
> > a few lines below, after the usage.
> >
> > Fixes: 76894f3e2f71 ("cifs: improve symlink handling for smb2+")
> > CC: Paulo Alcantara (SUSE) <pc@cjr.nz>
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/smb2inode.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
>
> Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>



-- 
Thanks,

Steve

[PATCH 3/3] cifs: do all necessary checks for credits within or before locking

[Shyam Prasad N]

All the server credits and in-flight info is protected by req_lock.
Once the req_lock is held, and we've determined that we have enough
credits to continue, this lock cannot be dropped till we've made the
changes to credits and in-flight count.

However, we used to drop the lock in order to avoid deadlock with
the recent srv_lock. This could cause the checks already made to be
invalidated.

Fixed it by moving the server status check to before locking req_lock.

Fixes: d7d7a66aacd6 ("cifs: avoid use of global locks for high contention data")
Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
---
 fs/smb/client/smb2ops.c   | 19 ++++++++++---------
 fs/smb/client/transport.c | 20 ++++++++++----------
 2 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
index 8e4900f6cd53..cb07ac32cf38 100644
--- a/fs/smb/client/smb2ops.c
+++ b/fs/smb/client/smb2ops.c
@@ -211,6 +211,16 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
 
 	spin_lock(&server->req_lock);
 	while (1) {
+		spin_unlock(&server->req_lock);
+
+		spin_lock(&server->srv_lock);
+		if (server->tcpStatus == CifsExiting) {
+			spin_unlock(&server->srv_lock);
+			return -ENOENT;
+		}
+		spin_unlock(&server->srv_lock);
+
+		spin_lock(&server->req_lock);
 		if (server->credits <= 0) {
 			spin_unlock(&server->req_lock);
 			cifs_num_waiters_inc(server);
@@ -221,15 +231,6 @@ smb2_wait_mtu_credits(struct TCP_Server_Info *server, unsigned int size,
 				return rc;
 			spin_lock(&server->req_lock);
 		} else {
-			spin_unlock(&server->req_lock);
-			spin_lock(&server->srv_lock);
-			if (server->tcpStatus == CifsExiting) {
-				spin_unlock(&server->srv_lock);
-				return -ENOENT;
-			}
-			spin_unlock(&server->srv_lock);
-
-			spin_lock(&server->req_lock);
 			scredits = server->credits;
 			/* can deadlock with reopen */
 			if (scredits <= 8) {
diff --git a/fs/smb/client/transport.c b/fs/smb/client/transport.c
index 0474d0bba0a2..f280502a2aee 100644
--- a/fs/smb/client/transport.c
+++ b/fs/smb/client/transport.c
@@ -522,6 +522,16 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
 	}
 
 	while (1) {
+		spin_unlock(&server->req_lock);
+
+		spin_lock(&server->srv_lock);
+		if (server->tcpStatus == CifsExiting) {
+			spin_unlock(&server->srv_lock);
+			return -ENOENT;
+		}
+		spin_unlock(&server->srv_lock);
+
+		spin_lock(&server->req_lock);
 		if (*credits < num_credits) {
 			scredits = *credits;
 			spin_unlock(&server->req_lock);
@@ -547,15 +557,6 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
 				return -ERESTARTSYS;
 			spin_lock(&server->req_lock);
 		} else {
-			spin_unlock(&server->req_lock);
-
-			spin_lock(&server->srv_lock);
-			if (server->tcpStatus == CifsExiting) {
-				spin_unlock(&server->srv_lock);
-				return -ENOENT;
-			}
-			spin_unlock(&server->srv_lock);
-
 			/*
 			 * For normal commands, reserve the last MAX_COMPOUND
 			 * credits to compound requests.
@@ -569,7 +570,6 @@ wait_for_free_credits(struct TCP_Server_Info *server, const int num_credits,
 			 * for servers that are slow to hand out credits on
 			 * new sessions.
 			 */
-			spin_lock(&server->req_lock);
 			if (!optype && num_credits == 1 &&
 			    server->in_flight > 2 * MAX_COMPOUND &&
 			    *credits <= MAX_COMPOUND) {
-- 
2.34.1

Re: [PATCH 1/3] cifs: log session id when a matching ses is not found

[Paulo Alcantara]

Shyam Prasad N <nspmangalore@gmail.com> writes:

> We do not log the session id in crypt_setup when a matching
> session is not found. Printing the session id helps debugging
> here. This change does just that.
>
> Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> ---
>  fs/smb/client/smb2ops.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> index a8bb9d00d33a..8e4900f6cd53 100644
> --- a/fs/smb/client/smb2ops.c
> +++ b/fs/smb/client/smb2ops.c
> @@ -4439,8 +4439,8 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
>  
>  	rc = smb2_get_enc_key(server, le64_to_cpu(tr_hdr->SessionId), enc, key);
>  	if (rc) {
> -		cifs_server_dbg(VFS, "%s: Could not get %scryption key\n", __func__,
> -			 enc ? "en" : "de");
> +		cifs_server_dbg(VFS, "%s: Could not get %scryption key. sid: 0x%llx\n", __func__,
> +			 enc ? "en" : "de", le64_to_cpu(tr_hdr->SessionId));

I think this should be FYI rather than VFS as it is usually fine to fail
on smb2_get_enc_key() while the session was reconnected, since the I/O
would be retried later and the current value of @tr_hdr->SessionId would
no longer match any existing session ids.  I have seen such messages
while running reconnect tests with 'seal' mount option.

Other than that, looks good.

Re: [PATCH 1/3] cifs: log session id when a matching ses is not found

[Shyam Prasad N]

On Fri, Jun 23, 2023 at 12:15 AM Paulo Alcantara <pc@manguebit.com> wrote:
>
> Shyam Prasad N <nspmangalore@gmail.com> writes:
>
> > We do not log the session id in crypt_setup when a matching
> > session is not found. Printing the session id helps debugging
> > here. This change does just that.
> >
> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com>
> > ---
> >  fs/smb/client/smb2ops.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/smb/client/smb2ops.c b/fs/smb/client/smb2ops.c
> > index a8bb9d00d33a..8e4900f6cd53 100644
> > --- a/fs/smb/client/smb2ops.c
> > +++ b/fs/smb/client/smb2ops.c
> > @@ -4439,8 +4439,8 @@ crypt_message(struct TCP_Server_Info *server, int num_rqst,
> >
> >       rc = smb2_get_enc_key(server, le64_to_cpu(tr_hdr->SessionId), enc, key);
> >       if (rc) {
> > -             cifs_server_dbg(VFS, "%s: Could not get %scryption key\n", __func__,
> > -                      enc ? "en" : "de");
> > +             cifs_server_dbg(VFS, "%s: Could not get %scryption key. sid: 0x%llx\n", __func__,
> > +                      enc ? "en" : "de", le64_to_cpu(tr_hdr->SessionId));
>
> I think this should be FYI rather than VFS as it is usually fine to fail
> on smb2_get_enc_key() while the session was reconnected, since the I/O
> would be retried later and the current value of @tr_hdr->SessionId would
> no longer match any existing session ids.  I have seen such messages
> while running reconnect tests with 'seal' mount option.
>
> Other than that, looks good.

Ack Paulo.
I'll send an updated patch after reducing this to FYI log.

-- 
Regards,
Shyam