Messages in this thread | | | Date | Mon, 8 Feb 2016 12:08:15 -0800 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH v2 2/2] debugfs: prevent access to removed files' private data |
| |
On Mon, Feb 08, 2016 at 09:00:05PM +0100, Nicolai Stange wrote: > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > > > On Mon, Feb 08, 2016 at 06:14:58PM +0100, Nicolai Stange wrote: > >> Greg Kroah-Hartman <gregkh@linuxfoundation.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? > > So this is a "yes", I should include these kind of fixes within this > series as [3/X], [4/X], ..., [X/X]?
Yes please.
> Last time I checked the tree (Nov.), there weren't any users of this > kind (debugfs file removal w/o module unload). > Obviously I missed ion though... I will recheck. > > > > >> 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. > > A quick check revealed that there are *many* such seqfile users. > > Since these would all get touched, I think it is better to postpone the > introduction of a debugfs_create_seqfile() to another series dedicated > to that?
Yes that would be a good idea.
thanks,
greg k-h
| |