linux-kernel - Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data

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

Message-ID: <20160208191653.GB9693@kroah.com>
Date:	Mon, 8 Feb 2016 11:16:53 -0800
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Nicolai Stange <nicstange@...il.com>
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	Jonathan Corbet <corbet@....net>, Jan Kara <jack@...e.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private
 data

On Mon, Feb 08, 2016 at 06:14:58PM +0100, Nicolai Stange wrote:
> Greg Kroah-Hartman <gregkh@...uxfoundation.org> writes:
> 
> > On Mon, Feb 08, 2016 at 04:03:27PM +0100, Nicolai Stange wrote:
> >> Upon return of debugfs_remove()/debugfs_remove_recursive(), it might
> >> still be attempted to access associated private file data through
> >> previously opened struct file objects. If that data has been freed by
> >> the caller of debugfs_remove*() in the meanwhile, the reading/writing
> >> process would either encounter a fault or, if the memory address in
> >> question has been reassigned again, unrelated data structures could get
> >> overwritten.
> >> 
> >> However, since debugfs files are seldomly removed, usually from module
> >> exit handlers only, the impact is very low.
> >> 
> >> Since debugfs_remove() and debugfs_remove_recursive() are already
> >> waiting for a SRCU grace period before returning to their callers,
> >> enclosing the access to private file data from ->read() and ->write()
> >> within a SRCU read-side critical section does the trick:
> >> - Introduce the debugfs_file_use_data_start() and
> >>   debugfs_file_use_data_finish() helpers which just enter and leave
> >>   a SRCU read-side critical section. The former also reports whether the
> >>   file is still alive, that is if d_delete() has _not_ been called on
> >>   the corresponding dentry.
> >> - Introduce the DEFINE_DEBUGFS_ATTRIBUTE() macro which is completely
> >>   equivalent to the DEFINE_SIMPLE_ATTRIBUTE() macro except that
> >>   ->read() and ->write are set to SRCU protecting wrappers around the
> >>   original simple_read() and simple_write() helpers.
> >> - Use that DEFINE_DEBUGFS_ATTRIBUTE() macro for all debugfs_create_*()
> >>   attribute creation variants where appropriate.
> >> - Manually introduce SRCU protection to the debugfs-predefined readers
> >>   and writers not covered by the above DEFINE_SIMPLE_ATTRIBUTE()->
> >>   DEFINE_DEBUGFS_ATTRIBUTE() replacement.
> >> 
> >> Finally, it should be worth to note that in the vast majority of cases
> >> where debugfs users are handing in a "custom" struct file_operations
> >> object to debugfs_create_file(), an attribute's associated data's
> >> lifetime is bound to the one of the containing module and thus,
> >> taking a reference on ->owner during file opening acts as a proxy here.
> >> There is no need to do a mass replace of DEFINE_SIMPLE_ATTRIBUTE() to
> >> DEFINE_DEBUGFS_ATTRIBUTE() outside of debugfs.
> >> 
> >> OTOH, new users of debugfs are encouraged to prefer the
> >> DEFINE_DEBUGFS_ATTRIBUTE() macro over DEFINE_SIMPLE_ATTRIBUTE() and it,
> >> as well as the needed read/write wrappers are made available globally.
> >> For new users implementing their own readers and writers, the lifetime
> >> management helpers debugfs_file_use_data_start() and
> >> debugfs_file_use_data_finish() are exported.
> >
> > Nice job.  One more request... :)
> >
> > Can you show how you would convert a subsystem to use these new
> > macros/calls to give a solid example of it in use outside of the debugfs
> > core?
> 
> You mean in the form of a patch [3/3] for an arbitrary subsystem other
> than debugfs? Or in the form of an update of
> Documentation/filesystems/debugfs.txt?

For an arbritary subsystem would be great.  Showing how this should be
used / converted tree-wide.

> In case you want to have a patch: for the DEFINE_DEBUGFS_ATTRIBUTE, I
> could simply abuse
>   drivers/staging/android/ion/ion.c
> as it has got a DEFINE_SIMPLE_ATTRIBUTE debug_shrink_fops passed to
> debugfs. In this particular case, it even looks like that this debugfs
> file can be removed through ion_client_destroy() without any module
> removal. Fixing this would be as easy as
> s/DEFINE_SIMPLE_ATTRIBUTE/DEFINE_DEBUGFS_ATTRIBUTE/.

Great, why wouldn't we do that for all users of debugfs that have this
type of interaction with it?

> Regarding a use case with custom made file_operations whose
> reader and writer are protected by the debugfs_file_use_data_*()
> helpers, I'm a little bit at a loss with: ion.c has got its custom
> 'debug_heap_fops', but in this case, it would probably be more
> appropriate to create a general debugfs_create_seqfile() centrally in
> debugfs.

ion is 'rough', but if enough people use seqfile in debugfs, yes, we
should provide a generic interface for it to make it easier to use so
they don't have to roll their own, and so they get the fixes you did
here for their code as well.

thanks,

greg k-h