Re: [PATCH 2/2] cifs: fix race in assemble_neg_contexts()

From: Enzo Matsumiya <ematsumiya@suse.de>
To: Paulo Alcantara <pc@cjr.nz>
Cc: smfrench@gmail.com, linux-cifs@vger.kernel.org
Subject: Re: [PATCH 2/2] cifs: fix race in assemble_neg_contexts()
Date: Thu, 29 Dec 2022 17:14:48 -0300	[thread overview]
Message-ID: <20221229201448.bzxqnsea52zcb4xw@suse.de> (raw)
In-Reply-To: <20221229153356.8221-2-pc@cjr.nz>

On 12/29, Paulo Alcantara wrote:
>Serialise access of TCP_Server_Info::hostname in
>assemble_neg_contexts() by holding the server's mutex otherwise it
>might end up accessing an already-freed hostname pointer from
>cifs_reconnect() or cifs_resolve_server().
>
>Signed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>

Couldn't reproduce this one as easy as the other one, but it makes sense
anyway.

Reviewed-by: Enzo Matsumiya <ematsumiya@suse.de>

>---
> fs/cifs/smb2pdu.c | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
>diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>index a5695748a89b..2c484d47c592 100644
>--- a/fs/cifs/smb2pdu.c
>+++ b/fs/cifs/smb2pdu.c
>@@ -541,9 +541,10 @@ static void
> assemble_neg_contexts(struct smb2_negotiate_req *req,
> 		      struct TCP_Server_Info *server, unsigned int *total_len)
> {
>-	char *pneg_ctxt;
>-	char *hostname = NULL;
> 	unsigned int ctxt_len, neg_context_count;
>+	struct TCP_Server_Info *pserver;
>+	char *pneg_ctxt;
>+	char *hostname;
>
> 	if (*total_len > 200) {
> 		/* In case length corrupted don't want to overrun smb buffer */
>@@ -574,8 +575,9 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> 	 * secondary channels don't have the hostname field populated
> 	 * use the hostname field in the primary channel instead
> 	 */
>-	hostname = CIFS_SERVER_IS_CHAN(server) ?
>-		server->primary_server->hostname : server->hostname;
>+	pserver = CIFS_SERVER_IS_CHAN(server) ? server->primary_server : server;
>+	cifs_server_lock(pserver);
>+	hostname = pserver->hostname;
> 	if (hostname && (hostname[0] != 0)) {
> 		ctxt_len = build_netname_ctxt((struct smb2_netname_neg_context *)pneg_ctxt,
> 					      hostname);
>@@ -584,6 +586,7 @@ assemble_neg_contexts(struct smb2_negotiate_req *req,
> 		neg_context_count = 3;
> 	} else
> 		neg_context_count = 2;
>+	cifs_server_unlock(pserver);
>
> 	build_posix_ctxt((struct smb2_posix_neg_context *)pneg_ctxt);
> 	*total_len += sizeof(struct smb2_posix_neg_context);
>-- 
>2.39.0
>