linux-kernel - Re: [PATCH 1/2] cifs: replace unnecessary use of list iterator variable with head

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <863A56C1-E99D-4298-AC32-94F9F1E24F1F@gmail.com>
Date:   Sat, 2 Apr 2022 00:57:21 +0200
From:   Jakob Koschel <jakobkoschel@...il.com>
To:     Steve French <smfrench@...il.com>
Cc:     Steve French <sfrench@...ba.org>,
        CIFS <linux-cifs@...r.kernel.org>,
        samba-technical <samba-technical@...ts.samba.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Mike Rapoport <rppt@...nel.org>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>,
        "Bos, H.J." <h.j.bos@...nl>
Subject: Re: [PATCH 1/2] cifs: replace unnecessary use of list iterator
 variable with head

Hey Steve,

> On 2. Apr 2022, at 00:44, Steve French <smfrench@...il.com> wrote:
> 
> It looks like this no longer applies cleanly.   Can you recheck
> fs/cifs/smb2pdu.c (function smb2_reconnect_server) and see if it
> applies now that it has changed e.g.

looks like this issue was already fixed in the meantime.
Feel free to ignore PATCH 1/2.

> 
> 
> /* allocate a dummy tcon struct used for reconnect */
> tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
> if (!tcon) {
> resched = true;
> list_for_each_entry_safe(ses, ses2, &tmp_ses_list, rlist) {
> list_del_init(&ses->rlist);
> cifs_put_smb_ses(ses);
> }
> goto done;
> }
> 
> You had this:
> 
> --- fs/cifs/smb2pdu.c
> +++ fs/cifs/smb2pdu.c
> @@ -3858,7 +3858,7 @@ void smb2_reconnect_server(struct work_struct *work)
>  tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
>  if (!tcon) {
>  resched = true;
> - list_del_init(&ses->rlist);
> + list_del_init(&pserver->smb_ses_list);
>  cifs_put_smb_ses(ses);
>  goto done;
>  }
> 
> On Fri, Apr 1, 2022 at 2:23 AM Jakob Koschel <jakobkoschel@...il.com> wrote:
>> 
>> When list_for_each_entry() completes the iteration over the whole list
>> without breaking the loop, the iterator variable will *always* be a
>> bogus pointer computed based on the head element.
>> 
>> To avoid type confusion use the actual list head directly instead of
>> the last iterator value.
>> 
>> Signed-off-by: Jakob Koschel <jakobkoschel@...il.com>
>> ---
>> fs/cifs/smb2pdu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index 7e7909b1ae11..4ac86b77a7c9 100644
>> --- a/fs/cifs/smb2pdu.c
>> +++ b/fs/cifs/smb2pdu.c
>> @@ -3858,7 +3858,7 @@ void smb2_reconnect_server(struct work_struct *work)
>>        tcon = kzalloc(sizeof(struct cifs_tcon), GFP_KERNEL);
>>        if (!tcon) {
>>                resched = true;
>> -               list_del_init(&ses->rlist);
>> +               list_del_init(&pserver->smb_ses_list);
>>                cifs_put_smb_ses(ses);
>>                goto done;
>>        }
>> 
>> base-commit: f82da161ea75dc4db21b2499e4b1facd36dab275
>> --
>> 2.25.1
>> 
> 
> 
> -- 
> Thanks,
> 
> Steve