[PATCH] loop: prevent get_user pages call from kernel thread(v2)

[PATCH] loop: prevent get_user pages call from kernel thread(v2)

[Dmitri Monakhov]

Yes... everybody know that it is bad to write from kernel thread, and it is
madness to do it with O_DIRECT. But occasionly file with O_DIRECT flag
may be passed to loop device via LOOP_SET_FD. So if file-system has't
address_space ops, or simply hide it like GFS, it is possible to kill kernel
via two lines program. In fact we can't effectively guard kernel space by
deny O_DIRECT in loop's code, because user space can set it via
fcntl(,F_SETFL,). Let's simply add sanity check mm related logic.	

Signed-off-by: Dmitri Monakhov <dmonakhov@openvz.org>
---
 drivers/block/loop.c |    5 +++++
 fs/direct-io.c       |    4 ++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index d3a25b0..01e2133 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -789,6 +789,11 @@ static int loop_set_fd(struct loop_device *lo, struct file *lo_file,
 	if (!(lo_file->f_mode & FMODE_WRITE))
 		lo_flags |= LO_FLAGS_READ_ONLY;
 
+	/* We can't use f_op->write with O_DIRECT from kernel thread. */
+	if (!(lo_flags & (LO_FLAGS_USE_AOPS | LO_FLAGS_READ_ONLY)) &&
+		file->f_mode & O_DIRECT)
+		goto out_putf;
+
 	set_device_ro(bdev, (lo_flags & LO_FLAGS_READ_ONLY) != 0);
 
 	lo->lo_blocksize = lo_blocksize;
diff --git a/fs/direct-io.c b/fs/direct-io.c
index 9606ee8..c383766 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -149,6 +149,10 @@ static int dio_refill_pages(struct dio *dio)
 	int ret;
 	int nr_pages;
 
+	if (unlikely(!current->mm)) {
+		WARN_ON_ONCE(1);
+		return -EINVAL;
+	}
 	nr_pages = min(dio->total_pages - dio->curr_page, DIO_PAGES);
 	ret = get_user_pages_fast(
 		dio->curr_user_address,		/* Where from? */
-- 
1.5.4.rc4

Re: [PATCH] loop: prevent get_user pages call from kernel thread(v2)

[Andi Kleen]

Dmitri Monakhov <dmonakhov@openvz.org> writes:

> Yes... everybody know that it is bad to write from kernel thread, and it is
> madness to do it with O_DIRECT. But occasionly file with O_DIRECT flag
> may be passed to loop device via LOOP_SET_FD. So if file-system has't
> address_space ops, or simply hide it like GFS, it is possible to kill kernel
> via two lines program. In fact we can't effectively guard kernel space by
> deny O_DIRECT in loop's code, because user space can set it via
> fcntl(,F_SETFL,). Let's simply add sanity check mm related logic.	

Wouldn't it be better if loop simply dup()ed the file descriptor
and then checked the flag?  Presumably other fd flags could
do bad things inside loop too.

-Andi

Re: [PATCH] loop: prevent get_user pages call from kernel thread(v2)

[Dmitri Monakhov]

Andi Kleen <andi@firstfloor.org> writes:

> Dmitri Monakhov <dmonakhov@openvz.org> writes:
>
>> Yes... everybody know that it is bad to write from kernel thread, and it is
>> madness to do it with O_DIRECT. But occasionly file with O_DIRECT flag
>> may be passed to loop device via LOOP_SET_FD. So if file-system has't
>> address_space ops, or simply hide it like GFS, it is possible to kill kernel
>> via two lines program. In fact we can't effectively guard kernel space by
>> deny O_DIRECT in loop's code, because user space can set it via
>> fcntl(,F_SETFL,). Let's simply add sanity check mm related logic.	
>
> Wouldn't it be better if loop simply dup()ed the file descriptor
> and then checked the flag?  Presumably other fd flags could
> do bad things inside loop too.
Off course this can't work because both fd refer to the same struct file.
man fcntl:
 File status flags
       Each open file description has certain associated status flags, ini-
       tialized  by  open(2) and possibly modified by fcntl(2).  Duplicated
       file descriptors (made with dup(2), fcntl(F_DUPFD),  fork(2),  etc.)
       refer  to  the  same  open file description, and thus share the same
       file status flags.
>
> -Andi