linux-kernel - Re: [PATCH] debugfs: Add proxy function for the mmap file operation

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: <87oa5gpcu1.fsf@gmail.com>
Date:	Fri, 29 Jul 2016 19:35:02 +0200
From:	Nicolai Stange <nicstange@...il.com>
To:	Liviu Dudau <Liviu.Dudau@....com>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Brian Starkey <Brian.Starkey@....com>,
	LKML <linux-kernel@...r.kernel.org>,
	Nicolai Stange <nicstange@...il.com>
Subject: Re: [PATCH] debugfs: Add proxy function for the mmap file operation

Liviu Dudau <Liviu.Dudau@....com> writes:

> Add proxy function for the mmap file_operations hook under the
> full_proxy_fops structure. This is useful for providing a custom
> mmap routine in a driver's debugfs implementation.

I guess you've got some specific use case for mmap() usage on some new
debugfs file in mind?

Currently, there exist only two mmap providers:
  drivers/staging/android/sync_debug.c
  kernel/kcov.c

Both don't suffer from the lack of mmap support in the debugfs full proxy
implementation because they don't use it -- their files never go away
and thus, can be (and are) created via debugfs_create_file_unsafe().

However, if you wish to have some mmapable debugfs file which *can* go
away, introducing mmap support in the debugfs full proxy is perfectly
valid. But please see below.

> Cc: Nicolai Stange <nicstange@...il.com>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@....com>
> ---
>  fs/debugfs/file.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index 592059f..d87148a 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -168,6 +168,10 @@ FULL_PROXY_FUNC(write, ssize_t, filp,
>  			loff_t *ppos),
>  		ARGS(filp, buf, size, ppos));
>  
> +FULL_PROXY_FUNC(mmap, int, filp,
> +		PROTO(struct file *filp, struct vm_area_struct *vma),
> +		ARGS(filp, vma));
> +

While this protects the call to ->mmap() itself against file removal
races, it doesn't protect anything possibly installed at vma->vm_ops
from that.

I'm fine with this as long as vma->vm_ops isn't set from ->mmap() ;).
At the very least, we should probably provide a Coccinelle script for
this. I'll try to put something together at the weekend or at the
beginning of next week (if you aren't faster).

Another option would be to add a check in the wrapping ->mmap() whether
the vma->vm_ops has been set from the wrapped ->mmap().
Greg, do you think such a runtime check would be a good thing to have?

Btw, it would certainly be possible to even support a custom vma->vm_ops
by proxying this one, too. However, we probably would have to SIGSEGV
userspace if ->fault() was called on a stale debugfs file. And since
nobody has asked for this feature yet, I don't think that it should be
implemented now.

Thanks,

Nicolai