lkml.org 
[lkml]   [2016]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [PATCH] aio: fix a user triggered use after free (and fix freeze protection of aio writes)
On Sat, Oct 29, 2016 at 8:20 AM, Christoph Hellwig <hch@lst.de> wrote:
>
> We can't as that would not fix the use after free (at least for the lockdep
> case - otherwise the call is a no-op). Once iter_op returns aio_complete
> might have dropped our reference to the file, and another thread might
> have closed the fd so that the fput from aio_complete was the last one.

I don't concpetually mind the patch per se, but the repeated

if (rw == WRITE) {
..
}

if (rw == WRITE) {
..
}

is just insane and makes the code less legible than it should be.

Also, honestly, make it use a helper: "aio_file_start_write()" and
"aio_file_end_write()" that has the comments and the lockdep games.

Because that patch is just too effing ugly.

Does something like the attached work for you guys?

Linus
fs/aio.c | 33 +++++++++++++++++++++++++++++----
include/linux/fs.h | 1 +
2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 1157e13a36d6..3f66331ef90c 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1066,6 +1066,27 @@ static struct kioctx *lookup_ioctx(unsigned long ctx_id)
return ret;
}

+/*
+ * We do file_start_write/file_end_write() to make sure
+ * we have filesystem freeze protection over the whole
+ * AIO write sequence, but to make sure that lockdep does
+ * not complain about the held lock when we return to
+ * userspace, we tell it that we release and reaquire the
+ * lock.
+ */
+static void aio_file_start_write(struct file *file)
+{
+ file_start_write(file);
+ __sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
+}
+
+static void aio_file_end_write(struct file *file)
+{
+ __sb_writers_acquired(file_inode(file)->i_sb, SB_FREEZE_WRITE);
+ file_end_write(file);
+}
+
+
/* aio_complete
* Called when the io request on the given iocb is complete.
*/
@@ -1078,6 +1099,9 @@ static void aio_complete(struct kiocb *kiocb, long res, long res2)
unsigned tail, pos, head;
unsigned long flags;

+ if (kiocb->ki_flags & IOCB_WRITE)
+ aio_file_end_write(kiocb->ki_filp);
+
/*
* Special case handling for sync iocbs:
* - events go directly into the iocb for fast handling
@@ -1460,13 +1484,14 @@ static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
return ret;
}

- if (rw == WRITE)
- file_start_write(file);
+ if (rw == WRITE) {
+ /* aio_complete() will end the write */
+ req->ki_flags |= IOCB_WRITE;
+ aio_file_start_write(file);
+ }

ret = iter_op(req, &iter);

- if (rw == WRITE)
- file_end_write(file);
kfree(iovec);
break;

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 16d2b6e874d6..db600e9bb1b4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -321,6 +321,7 @@ struct writeback_control;
#define IOCB_HIPRI (1 << 3)
#define IOCB_DSYNC (1 << 4)
#define IOCB_SYNC (1 << 5)
+#define IOCB_WRITE (1 << 6)

struct kiocb {
struct file *ki_filp;
\
 
 \ /
  Last update: 2016-10-29 19:48    [W:0.094 / U:0.792 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site