Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1736613 > unrolled thread

[PATCH] debugfs: Add check for module parameter name

Back to article view | Back to linux.kernel

Contents

  [PATCH] debugfs: Add check for module parameter name Arvind Yadav <arvind.yadav.cs@gmail.com> - 2017-09-21 14:20 +0200
    Re: [PATCH] debugfs: Add check for module parameter name Al Viro <viro@ZenIV.linux.org.uk> - 2017-09-21 14:50 +0200
      Re: [PATCH] debugfs: Add check for module parameter name arvind <arvind.yadav.cs@gmail.com> - 2017-09-21 18:50 +0200
        Re: [PATCH] debugfs: Add check for module parameter name Al Viro <viro@ZenIV.linux.org.uk> - 2017-09-21 19:30 +0200

#1736613 — [PATCH] debugfs: Add check for module parameter name

Here, start_creating() is calling by debugfs_create_dir()
and debugfs_create_automount(). driver can pass name as NULL in
debugfs_create_dir and debugfs_create_automount. So we need to
add check for 'name'.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 fs/debugfs/inode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index c59f015..aa5988d 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -292,6 +292,9 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
 	if (IS_ERR(parent))
 		return parent;
 
+	if (!name)
+		return ERR_PTR(-ENOMEM);
+
 	error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
 			      &debugfs_mount_count);
 	if (error)
-- 
1.9.1

[toc] | [next] | [standalone]

#1736634

On Thu, Sep 21, 2017 at 05:46:54PM +0530, Arvind Yadav wrote:
> Here, start_creating() is calling by debugfs_create_dir()
> and debugfs_create_automount(). driver can pass name as NULL in
> debugfs_create_dir and debugfs_create_automount. So we need to
> add check for 'name'.

Huh?  "Driver can pass any kind of crap pointer when calling this
function, so let's check if that crap happens to be NULL and bail
out in that particular case"?  Or am I misreading that?

Do you have any in-tree examples, or is that about some out-of-tree
code that needs to be saved from itself?

[toc] | [prev] | [next] | [standalone]

#1736848

Hi,

On Thursday 21 September 2017 06:14 PM, Al Viro wrote:
> On Thu, Sep 21, 2017 at 05:46:54PM +0530, Arvind Yadav wrote:
>> Here, start_creating() is calling by debugfs_create_dir()
>> and debugfs_create_automount(). driver can pass name as NULL in
>> debugfs_create_dir and debugfs_create_automount. So we need to
>> add check for 'name'.
> Huh?  "Driver can pass any kind of crap pointer when calling this
> function, so let's check if that crap happens to be NULL and bail
> out in that particular case"?  Or am I misreading that?
Your are correct.
>
> Do you have any in-tree examples, or is that about some out-of-tree
> code that needs to be saved from itself?
>
Please check "drivers/base/power/opp/debugfs.c"

static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
                                       struct opp_table *opp_table,
                                       struct dentry *pdentry)
{
         struct dentry *d;
         int i;
         char *name;

         for (i = 0; i < opp_table->regulator_count; i++) {
                 name = kasprintf(GFP_KERNEL, "supply-%d", i);

                 /* Create per-opp directory */
                 d = debugfs_create_dir(name, pdentry);

                 kfree(name);
.
.
.
}

kasprintf() can fail here and It can return NULL.
In this case,  We are passing NULL value to debugfs_create_dir().
I know, we will have to handle kasprintf() first instead of adding
NULL check in start_creating(). I have seen few driver where they have done
similar kind of implementation. Also I am adding check for kasprintf.

~arvind

[toc] | [prev] | [next] | [standalone]

#1736961

On Thu, Sep 21, 2017 at 10:17:46PM +0530, arvind wrote:
> Hi,
> 
> On Thursday 21 September 2017 06:14 PM, Al Viro wrote:
> > On Thu, Sep 21, 2017 at 05:46:54PM +0530, Arvind Yadav wrote:
> > > Here, start_creating() is calling by debugfs_create_dir()
> > > and debugfs_create_automount(). driver can pass name as NULL in
> > > debugfs_create_dir and debugfs_create_automount. So we need to
> > > add check for 'name'.
> > Huh?  "Driver can pass any kind of crap pointer when calling this
> > function, so let's check if that crap happens to be NULL and bail
> > out in that particular case"?  Or am I misreading that?
> Your are correct.
> > 
> > Do you have any in-tree examples, or is that about some out-of-tree
> > code that needs to be saved from itself?
> > 
> Please check "drivers/base/power/opp/debugfs.c"
> 
> static bool opp_debug_create_supplies(struct dev_pm_opp *opp,
>                                       struct opp_table *opp_table,
>                                       struct dentry *pdentry)
> {
>         struct dentry *d;
>         int i;
>         char *name;
> 
>         for (i = 0; i < opp_table->regulator_count; i++) {
>                 name = kasprintf(GFP_KERNEL, "supply-%d", i);
> 
>                 /* Create per-opp directory */
>                 d = debugfs_create_dir(name, pdentry);
> 
>                 kfree(name);

Umm...  Looks like crap, to be honest.  And not just that function -
if anything in there fails to create a file, the thing leaks all
kinds of garbage.

AFAICS, that code has never been tested (and probably not thought
through in the first place) in case of allocation failures.  So
much that an oops might be a mercy - at least then somebody might
consider getting it into sane shape...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web