linux-kernel - [RFC PATCH 0/3] debugfs: make __debugfs

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

Date:	Thu, 10 Dec 2015 13:47:11 +0100
From:	Roman Pen <r.peniaev@...il.com>
To:	unlisted-recipients:; (no To-header on input)
Cc:	Roman Pen <r.peniaev@...il.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org
Subject: [RFC PATCH 0/3] debugfs: make __debugfs_remove wait for dentry release

Hello.

Recently I observed that all over the sources people expect that debugfs_remove()
should behave as a barrier, hence file operations won't be invoked after it is
called and it is safe to free remain memory.

But by default debugfs_remove() does not guarantee that dentry will be released,
and after this call fops->open/read/write can still be invoked if someone holds
the reference on dentry because of successfull lookup.

For example here is the grep output:

   *** drivers/block/pktcdvd.c:
       pkt_debugfs_dev_remove[489]    debugfs_remove(pd->dfs_f_info);
       pkt_debugfs_dev_remove[490]    debugfs_remove(pd->dfs_d_root);

   *** drivers/char/virtio_console.c:
       unplug_port[1595]              debugfs_remove(port->debugfs_file);

   *** drivers/crypto/qat/qat_common/adf_cfg.c:
       adf_cfg_dev_remove[187]        debugfs_remove(dev_cfg_data->debug);

   *** drivers/gpu/drm/drm_debugfs.c:
       drm_debugfs_remove_files[203]  debugfs_remove(tmp->dent);

  .... and more and more and more ...

where people do the following sequence:

      debugfs_remove(dev->debugfs_dentry);
      kfree(dev);

and 'dev' pointer was passed to 'debugfs_create_file("my_dev", , dev, dev_fops)',
so any access to 'dev' in file operations can lead to usage-after-free.

In this patch __debugfs_remove() waits for last dentry release callback.

BUT! I am not sure that nobody tries to remove the dentry from it's own file
operation (dentry suicide).  And if so - deadlock will happen.

Probably, dentry_remove_self() should be implemented for such cases, which is
similar to sysfs_remove_file_self().  But for now I do not want to add new
function which can be useless in the nearest future.

Also I did a fix for automount inodes and increased i_nlink references because
of WARNING at fs/inode.c:273 drop_nlink.

Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org

Roman Pen (3):
  debugfs: fix automount inode i_nlink references
  debugfs: put private data to i_private for automount inode
  debugfs: make __debugfs_remove wait for dentry release

 fs/debugfs/inode.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 7 deletions(-)

-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/