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

From: Steve French <smfrench@gmail.com>
To: Enzo Matsumiya <ematsumiya@suse.de>
Cc: Paulo Alcantara <pc@cjr.nz>, linux-cifs@vger.kernel.org
Subject: Re: [PATCH 2/2] cifs: fix race in assemble_neg_contexts()
Date: Wed, 4 Jan 2023 00:14:15 -0600	[thread overview]
Message-ID: <CAH2r5mu+C0kxvdg8XEyYJYaZQc0Q2EzddtwwLPmzR4=djsQmOw@mail.gmail.com> (raw)
In-Reply-To: <CAH2r5mso6PLpKpVxtcUHW4RQ1jc-Tmj5ALOEba2z+40uSDf0JA@mail.gmail.com>

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


-- 
Thanks,

Steve