linux-kernel - RE: maybe resource leak in security/selinux/selinuxfs.c

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]

Date:   Sun, 5 Aug 2018 09:37:43 +0000
From:   Nixiaoming <nixiaoming@...wei.com>
To:     Paul Moore <paul@...l-moore.com>
CC:     "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "serge@...lyn.com" <serge@...lyn.com>,
        James Morris <jmorris@...ei.org>,
        Eric Paris <eparis@...isplace.org>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        Lizefan <lizefan@...wei.com>, "miaoxie (A)" <miaoxie@...wei.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-security-module@...r.kernel.org" 
        <linux-security-module@...r.kernel.org>,
        "selinux@...ho.nsa.gov" <selinux@...ho.nsa.gov>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: RE: maybe resource leak in security/selinux/selinuxfs.c

On Saturday, August 04, 2018 3:01 AM Paul Moore <paul@...l-moore.com> wrote:
>On Wed, Aug 1, 2018 at 5:39 AM Nixiaoming <nixiaoming@...wei.com> wrote:
>>
>> advisory:
>> 1 After creating dentry in d_alloc_name, should I call dput to release resources before the exception exit?
>> 2 After calling the new_inode to create an inode, should the inode resource be released before the exception exit?
>>
>> If the dentry and inode resources need to be actively released, there are multiple resource leaks in security/selinux/selinuxfs.c.
>
>Hello.  Sorry for the delay in responding, comments inline ...
>
>> Example:
>> Linux master branch v4.18-rc5
>> The function sel_make_avc_files in security/selinux/selinuxfs.c.
>>
>> 1566 static int sel_make_avc_files(struct dentry *dir)
>> .......
>> 1580     for (i = 0; i < ARRAY_SIZE(files); i++) {
>> 1581         struct inode *inode;
>> 1582         struct dentry *dentry;
>> 1583
>> 1584         dentry = d_alloc_name(dir, files[i].name);
>> 1585         if (!dentry)
>> /*Resource leak: when i!=0, the release action of dentry and inode resources is missing*/
>
>I don't understand the leak in this case?  If d_alloc_name() fails on
>a partially constructed /sys/fs/selinux/avc directory the previously
>allocated dentry/inodes are not lost or leaked, d_alloc_name()
>attached them to the parent dentry.
>
Sorry, I didn't notice the directory release code in sel_kill_sb before.

>> 1586             return -ENOMEM;
>> 1587
>> 1588         inode = sel_make_inode(dir->d_sb, S_IFREG|files[i].mode);
>> 1589         if (!inode)
>> /*Resource leak: missing dput(dentry)*/
>> /*Resource leak: when i!=0, the release action of dentry and inode resources is missing*/
>
>Yes, in this case it does look like we are missing a call to dput().
>Would you like to submit a patch to fix this as well as the others in
>selinuxfs.c?
>
Thank you very much for your review. 
I will submit the patch as soon as possible.

>> 1590             return -ENOMEM;
>> 1591
>> 1592         inode->i_fop = files[i].ops;
>> 1593         inode->i_ino = ++fsi->last_ino;
>> 1594         d_add(dentry, inode);
>> 1595     }
>> 1596
>> 1597     return 0;
>> 1598 }
>>
>> There are similar resource leaking functions:
>> Sel_make_bools
>> Sel_make_avc_files
>> Sel_make_initcon_files
>> Sel_make_perm_files
>> Sel_make_class_dir_entries
>> Sel_make_policycap
>> Sel_fill_super
>> Sel_make_policy_nodes
>> Sel_make_classes
>
>-- 
>paul moore
>www.paul-moore.com
>

Thanks