linux-kernel - Re: build_path_from_dentry_optional_prefix() may schedule from invalid context

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKywueRd4d_fojGL+n4BisoibhgkYfN9Wyc_+0=-1sarz4-HZw@mail.gmail.com>
Date:   Thu, 19 Sep 2019 17:11:54 -0700
From:   Pavel Shilovsky <piastryyy@...il.com>
To:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Cc:     Steve French <stfrench@...rosoft.com>,
        Ronnie Sahlberg <lsahlber@...hat.com>,
        linux-cifs <linux-cifs@...r.kernel.org>,
        Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: build_path_from_dentry_optional_prefix() may schedule from
 invalid context

ср, 28 авг. 2019 г. в 22:02, Sergey Senozhatsky
<sergey.senozhatsky.work@...il.com>:
>
> Hello,
>
> Looking at commit "cifs: create a helper to find a writeable handle
> by path name":
>
> ->open_file_lock scope is atomic context, while build_path_from_dentry()
> can schedule - kmalloc(GFP_KERNEL)
>
>        spin_lock(&tcon->open_file_lock);
>        list_for_each(tmp, &tcon->openFileList) {
>                cfile = list_entry(tmp, struct cifsFileInfo,
>                             tlist);
>                full_path = build_path_from_dentry(cfile->dentry);
>                if (full_path == NULL) {
>                        spin_unlock(&tcon->open_file_lock);
>                        return -ENOMEM;
>                }
>                if (strcmp(full_path, name)) {
>                        kfree(full_path);
>                        continue;
>                }
>                kfree(full_path);
>
>                cinode = CIFS_I(d_inode(cfile->dentry));
>                spin_unlock(&tcon->open_file_lock);
>                return cifs_get_writable_file(cinode, 0, ret_file);
>        }
>
>        spin_unlock(&tcon->open_file_lock);
>
> Additionally, kfree() can (and should) be done outside of
> ->open_file_lock scope.
>
>         -ss

Good catch. I think we should have another version of
build_path_from_dentry() which takes pre-allocated (probably on stack)
full_path as an argument. This would allow us to avoid allocations
under the spin lock.
--
Best regards,
Pavel Shilovsky