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

[Greg Kroah-Hartman]

On Tue, Dec 01, 2015 at 12:26:41AM +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.
> 
> Signed-off-by: Nicolai Stange <nicstange@xxxxxxxxx>
> ---
>  Applicable to the Linus tree.
>  The second of the two patches depends on the first one

This needs a refresh as well.

And:

> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index 8ae2e1a..37222c9 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -31,6 +31,7 @@
>  #define DEBUGFS_DEFAULT_MODE	0700
>  
>  DEFINE_SRCU(debugfs_srcu);
> +EXPORT_SYMBOL_GPL(debugfs_srcu);

You shouldn't need to export this, because:

>  static struct vfsmount *debugfs_mount;
>  static int debugfs_mount_count;
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index f8c7494..ba1a299 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -20,6 +20,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/srcu.h>
> +#include <linux/compiler.h>
>  
>  struct device;
>  struct file_operations;
> @@ -128,6 +129,71 @@ ssize_t debugfs_read_file_bool(struct file *file, char __user *user_buf,
>  ssize_t debugfs_write_file_bool(struct file *file, const char __user *user_buf,
>  				size_t count, loff_t *ppos);
>  
> +int __debugfs_file_use_data_start(struct file *file, int *srcu_idx)
> +	__acquires(&debugfs_srcu);
> +
> +/**
> + * debugfs_file_use_data_start - mark the beginning of file data access
> + * @file: the file object whose data is being accessed.
> + * @srcu_idx: a pointer to some memory to store a SRCU index in.
> + *
> + * Up to a matching call to debugfs_file_use_data_finish(), any
> + * successive call into the file removing functions debugfs_remove()
> + * and debugfs_remove_recursive() will block. Since associated private
> + * file data may only get freed after a successful return of any of
> + * the removal functions, you may safely access it after a successful
> + * call to debugfs_file_use_data_start() without worrying about
> + * lifetime issues.
> + *
> + * If -%EIO is returned, the file has already been removed and thus,
> + * it is not safe to access any of its data. If, on the other hand,
> + * it is allowed to access the file data, zero is returned.
> + *
> + * Regardless of the return code, any call to
> + * debugfs_file_use_data_start() must be followed by a matching call
> + * to debugfs_file_use_data_finish().
> + */
> +static inline int debugfs_file_use_data_start(struct file *file, int *srcu_idx)
> +	__acquires(&debugfs_srcu)
> +{
> +	return __debugfs_file_use_data_start(file, srcu_idx);
> +}
> +
> +/**
> + * debugfs_file_use_data_finish - mark the end of file data access
> + * @srcu_idx: the SRCU index "created" by a former call to
> + *            debugfs_file_use_data_start().
> + *
> + * Allow any ongoing concurrent call into debugfs_remove() or
> + * debugfs_remove_recursive() blocked by a former call to
> + * debugfs_file_use_data_start() to proceed and return to its caller.
> + */
> +static inline void debugfs_file_use_data_finish(int srcu_idx)
> +	__releases(&debugfs_srcu)
> +{
> +	srcu_read_unlock(&debugfs_srcu, srcu_idx);
> +}

These don't need to be in debugfs.h as no one calls them except the
internal debugfs code, so you can put it in your new internal.h file, or
just make them a "real" function call, no need to inline it, there's no
speed issue here.

> +
> +ssize_t debugfs_attr_read(struct file *file, char __user *buf,
> +			 size_t len, loff_t *ppos);
> +ssize_t debugfs_attr_write(struct file *file, const char __user *buf,
> +			  size_t len, loff_t *ppos);
> +
> +#define DEFINE_DEBUGFS_ATTRIBUTE(__fops, __get, __set, __fmt)		\
> +static int __fops ## _open(struct inode *inode, struct file *file)	\
> +{									\
> +	__simple_attr_check_format(__fmt, 0ull);			\
> +	return simple_attr_open(inode, file, __get, __set, __fmt);	\
> +}									\
> +static const struct file_operations __fops = {				\
> +	.owner	 = THIS_MODULE,					\
> +	.open	 = __fops ## _open,					\
> +	.release = simple_attr_release,				\
> +	.read	 = debugfs_attr_read,					\
> +	.write	 = debugfs_attr_write,					\
> +	.llseek	 = generic_file_llseek,			\
> +}

And does this need to be in debugfs.h as well?

thanks,

greg k-h