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.
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*/
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*/
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
On Wed, Aug 1, 2018 at 5:39 AM Nixiaoming <[email protected]> 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.
> 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?
> 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
http://www.paul-moore.com
On Saturday, August 04, 2018 3:01 AM Paul Moore <[email protected]> wrote:
>On Wed, Aug 1, 2018 at 5:39 AM Nixiaoming <[email protected]> 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
>http://www.paul-moore.com
>
Thanks