* maybe resource leak in security/selinux/selinuxfs.c
@ 2018-08-01 9:23 Nixiaoming
2018-08-03 19:01 ` Paul Moore
0 siblings, 1 reply; 3+ messages in thread
From: Nixiaoming @ 2018-08-01 9:23 UTC (permalink / raw)
To: viro, serge, jmorris, eparis, sds, paul, Lizefan, miaoxie (A)
Cc: linux-kernel, linux-security-module, selinux, linux-fsdevel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: maybe resource leak in security/selinux/selinuxfs.c
2018-08-01 9:23 maybe resource leak in security/selinux/selinuxfs.c Nixiaoming
@ 2018-08-03 19:01 ` Paul Moore
2018-08-05 9:37 ` Nixiaoming
0 siblings, 1 reply; 3+ messages in thread
From: Paul Moore @ 2018-08-03 19:01 UTC (permalink / raw)
To: nixiaoming
Cc: viro, serge, James Morris, Eric Paris, Stephen Smalley, lizefan,
miaoxie, linux-kernel, linux-security-module, selinux,
linux-fsdevel
On Wed, Aug 1, 2018 at 5:39 AM Nixiaoming <nixiaoming@huawei.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.
> 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
www.paul-moore.com
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: maybe resource leak in security/selinux/selinuxfs.c
2018-08-03 19:01 ` Paul Moore
@ 2018-08-05 9:37 ` Nixiaoming
0 siblings, 0 replies; 3+ messages in thread
From: Nixiaoming @ 2018-08-05 9:37 UTC (permalink / raw)
To: Paul Moore
Cc: viro, serge, James Morris, Eric Paris, Stephen Smalley, Lizefan,
miaoxie (A),
linux-kernel, linux-security-module, selinux, linux-fsdevel
On Saturday, August 04, 2018 3:01 AM Paul Moore <paul@paul-moore.com> wrote:
>On Wed, Aug 1, 2018 at 5:39 AM Nixiaoming <nixiaoming@huawei.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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-08-05 11:41 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-01 9:23 maybe resource leak in security/selinux/selinuxfs.c Nixiaoming
2018-08-03 19:01 ` Paul Moore
2018-08-05 9:37 ` Nixiaoming
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).