diff mbox series

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

Message ID 20221229153356.8221-2-pc@cjr.nz
State New
Headers show
Series [1/2] cifs: ignore ipc reconnect failures during dfs failover | expand

Commit Message

Paulo Alcantara Dec. 29, 2022, 3:33 p.m. UTC 
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>
---
 fs/cifs/smb2pdu.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Enzo Matsumiya Dec. 29, 2022, 8:14 p.m. UTC | #1 
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
>
Steve French Jan. 4, 2023, 6:14 a.m. UTC | #2 
On Wed, Jan 4, 2023 at 12:12 AM Steve French <smfrench@gmail.com> wrote:
>
> merged into cifs-2.6.git for-next
>
> On Thu, Dec 29, 2022 at 2:14 PM Enzo Matsumiya <ematsumiya@suse.de> wrote:
>>
>> 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
>> >
>
>
>
> --
> Thanks,
>
> Steve
diff mbox series

Patch

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);