Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Christoph Hellwig]

Al,

any chance to send this user triggerable use after free on to Linus?

On Sun, Oct 16, 2016 at 07:51:22AM +0200, Christoph Hellwig wrote:
> From: Jan Kara 
> 
> Currently we dropped freeze protection of aio writes just after IO was
> submitted. Thus aio write could be in flight while the filesystem was
> frozen and that could result in unexpected situation like aio completion
> wanting to convert extent type on frozen filesystem. Testcase from
> Dmitry triggering this is like:
> 
> for ((i=0;i<60;i++));do fsfreeze -f /mnt ;sleep 1;fsfreeze -u /mnt;done &
> fio --bs=4k --ioengine=libaio --iodepth=128 --size=1g --direct=1 \
> --runtime=60 --filename=/mnt/file --name=rand-write --rw=randwrite
> 
> Fix the problem by dropping freeze protection only once IO is completed
> in aio_complete().
> 
> [hch: The above was the changelog of the original patch from Jan.
>  It turns out that it fixes something even more important - a use
>  after free of the file structucture given that the direct I/O
>  code calls fput and potentially drops the last reference to it in
>  aio_complete.  Together with two racing threads and a zero sized
>  I/O this seems easily exploitable]
> 
> Reported-by: Dmitry Monakhov 
> Signed-off-by: Jan Kara 
> [hch: switch to use __sb_writers_acquired and file_inode(file),
>   updated changelog]
> Signed-off-by: Christoph Hellwig 
> ---
>  fs/aio.c   | 28 +---
>  include/linux/fs.h |  1 +
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 1157e13..bf315cd 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1078,6 +1078,17 @@ static void aio_complete(struct kiocb *kiocb, long 
> res, long res2)
>   unsigned tail, pos, head;
>   unsigned long   flags;
>  
> + if (kiocb->ki_flags & IOCB_WRITE) {
> + struct file *file = kiocb->ki_filp;
> +
> + /*
> +  * Tell lockdep we inherited freeze protection from submission
> +  * thread.
> +  */
> + __sb_writers_acquired(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> + file_end_write(file);
> + }
> +
>   /*
>* Special case handling for sync iocbs:
>*  - events go directly into the iocb for fast handling
> @@ -1460,13 +1471,24 @@ static ssize_t aio_run_iocb(struct kiocb *req, 
> unsigned opcode,
>   return ret;
>   }
>  
> - if (rw == WRITE)
> + if (rw == WRITE) {
>   file_start_write(file);
> + req->ki_flags |= IOCB_WRITE;
> + }
> +
> + if (rw == WRITE) {
> + /*
> +  * We release freeze protection in aio_complete(). Fool
> +  * lockdep by telling it the lock got released so that
> +  * it doesn't complain about held lock when we return
> +  * to userspace.
> +  */
> + __sb_writers_release(file_inode(file)->i_sb,
> + SB_FREEZE_WRITE);
> + }
>  
>   ret = iter_op(req, );
>  
> - if (rw == WRITE)
> - file_end_write(file);
>   kfree(iovec);
>   break;
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 16d2b6e..db600e9 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;
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Christoph Hellwig]

Al,

any chance to send this user triggerable use after free on to Linus?

On Sun, Oct 16, 2016 at 07:51:22AM +0200, Christoph Hellwig wrote:
> From: Jan Kara 
> 
> Currently we dropped freeze protection of aio writes just after IO was
> submitted. Thus aio write could be in flight while the filesystem was
> frozen and that could result in unexpected situation like aio completion
> wanting to convert extent type on frozen filesystem. Testcase from
> Dmitry triggering this is like:
> 
> for ((i=0;i<60;i++));do fsfreeze -f /mnt ;sleep 1;fsfreeze -u /mnt;done &
> fio --bs=4k --ioengine=libaio --iodepth=128 --size=1g --direct=1 \
> --runtime=60 --filename=/mnt/file --name=rand-write --rw=randwrite
> 
> Fix the problem by dropping freeze protection only once IO is completed
> in aio_complete().
> 
> [hch: The above was the changelog of the original patch from Jan.
>  It turns out that it fixes something even more important - a use
>  after free of the file structucture given that the direct I/O
>  code calls fput and potentially drops the last reference to it in
>  aio_complete.  Together with two racing threads and a zero sized
>  I/O this seems easily exploitable]
> 
> Reported-by: Dmitry Monakhov 
> Signed-off-by: Jan Kara 
> [hch: switch to use __sb_writers_acquired and file_inode(file),
>   updated changelog]
> Signed-off-by: Christoph Hellwig 
> ---
>  fs/aio.c   | 28 +---
>  include/linux/fs.h |  1 +
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 1157e13..bf315cd 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1078,6 +1078,17 @@ static void aio_complete(struct kiocb *kiocb, long 
> res, long res2)
>   unsigned tail, pos, head;
>   unsigned long   flags;
>  
> + if (kiocb->ki_flags & IOCB_WRITE) {
> + struct file *file = kiocb->ki_filp;
> +
> + /*
> +  * Tell lockdep we inherited freeze protection from submission
> +  * thread.
> +  */
> + __sb_writers_acquired(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> + file_end_write(file);
> + }
> +
>   /*
>* Special case handling for sync iocbs:
>*  - events go directly into the iocb for fast handling
> @@ -1460,13 +1471,24 @@ static ssize_t aio_run_iocb(struct kiocb *req, 
> unsigned opcode,
>   return ret;
>   }
>  
> - if (rw == WRITE)
> + if (rw == WRITE) {
>   file_start_write(file);
> + req->ki_flags |= IOCB_WRITE;
> + }
> +
> + if (rw == WRITE) {
> + /*
> +  * We release freeze protection in aio_complete(). Fool
> +  * lockdep by telling it the lock got released so that
> +  * it doesn't complain about held lock when we return
> +  * to userspace.
> +  */
> + __sb_writers_release(file_inode(file)->i_sb,
> + SB_FREEZE_WRITE);
> + }
>  
>   ret = iter_op(req, );
>  
> - if (rw == WRITE)
> - file_end_write(file);
>   kfree(iovec);
>   break;
>  
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 16d2b6e..db600e9 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;
> -- 
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
---end quoted text---

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Christoph Hellwig]

On Mon, Oct 17, 2016 at 04:04:00PM -0400, Jeff Moyer wrote:
> >> Could we just change percpu_rw_semaphore->read_count to be a signed
> >> integer?  The down_write path sums up the counters from all cpus...
> >
> > To what point?
> 
> Duh, nevermind.  You're right, it should work as-is.

Ok, thanks.  That also explains why I didn't see any splat in
xfstests..

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Christoph Hellwig]

On Mon, Oct 17, 2016 at 04:04:00PM -0400, Jeff Moyer wrote:
> >> Could we just change percpu_rw_semaphore->read_count to be a signed
> >> integer?  The down_write path sums up the counters from all cpus...
> >
> > To what point?
> 
> Duh, nevermind.  You're right, it should work as-is.

Ok, thanks.  That also explains why I didn't see any splat in
xfstests..

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Jeff Moyer]

Christoph Hellwig  writes:

> From: Jan Kara 
>
> Currently we dropped freeze protection of aio writes just after IO was
> submitted. Thus aio write could be in flight while the filesystem was
> frozen and that could result in unexpected situation like aio completion
> wanting to convert extent type on frozen filesystem. Testcase from
> Dmitry triggering this is like:
>
> for ((i=0;i<60;i++));do fsfreeze -f /mnt ;sleep 1;fsfreeze -u /mnt;done &
> fio --bs=4k --ioengine=libaio --iodepth=128 --size=1g --direct=1 \
> --runtime=60 --filename=/mnt/file --name=rand-write --rw=randwrite
>
> Fix the problem by dropping freeze protection only once IO is completed
> in aio_complete().
>
> [hch: The above was the changelog of the original patch from Jan.
>  It turns out that it fixes something even more important - a use
>  after free of the file structucture given that the direct I/O
>  code calls fput and potentially drops the last reference to it in
>  aio_complete.  Together with two racing threads and a zero sized
>  I/O this seems easily exploitable]
>
> Reported-by: Dmitry Monakhov 
> Signed-off-by: Jan Kara 
> [hch: switch to use __sb_writers_acquired and file_inode(file),
>   updated changelog]
> Signed-off-by: Christoph Hellwig 

Reviewed-by: Jeff Moyer 

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Jeff Moyer]

Christoph Hellwig  writes:

> From: Jan Kara 
>
> Currently we dropped freeze protection of aio writes just after IO was
> submitted. Thus aio write could be in flight while the filesystem was
> frozen and that could result in unexpected situation like aio completion
> wanting to convert extent type on frozen filesystem. Testcase from
> Dmitry triggering this is like:
>
> for ((i=0;i<60;i++));do fsfreeze -f /mnt ;sleep 1;fsfreeze -u /mnt;done &
> fio --bs=4k --ioengine=libaio --iodepth=128 --size=1g --direct=1 \
> --runtime=60 --filename=/mnt/file --name=rand-write --rw=randwrite
>
> Fix the problem by dropping freeze protection only once IO is completed
> in aio_complete().
>
> [hch: The above was the changelog of the original patch from Jan.
>  It turns out that it fixes something even more important - a use
>  after free of the file structucture given that the direct I/O
>  code calls fput and potentially drops the last reference to it in
>  aio_complete.  Together with two racing threads and a zero sized
>  I/O this seems easily exploitable]
>
> Reported-by: Dmitry Monakhov 
> Signed-off-by: Jan Kara 
> [hch: switch to use __sb_writers_acquired and file_inode(file),
>   updated changelog]
> Signed-off-by: Christoph Hellwig 

Reviewed-by: Jeff Moyer 

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Jeff Moyer]

Peter Zijlstra  writes:

> On Mon, Oct 17, 2016 at 03:40:24PM -0400, Jeff Moyer wrote:
>> Christoph Hellwig  writes:
>> 
>> > On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
>> >> This ends up being a call to __sb_end_write:
>> >> 
>> >> void __sb_end_write(struct super_block *sb, int level)
>> >> {
>> >> percpu_up_read(sb->s_writers.rw_sem + level-1);
>> >> }
>> >> 
>> >> Nothing guarantees that submission and completion happen on the same
>> >> CPU.  Is this safe?
>> >
>> > Good point.  From my reading of the percpu_rwsem implementation it
>> > is not safe to release it from a different CPU.  Which makes me
>> > wonder how we can protect aio writes properly here..
>> 
>> Could we just change percpu_rw_semaphore->read_count to be a signed
>> integer?  The down_write path sums up the counters from all cpus...
>
> To what point?

Duh, nevermind.  You're right, it should work as-is.

-Jeff

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Jeff Moyer]

Peter Zijlstra  writes:

> On Mon, Oct 17, 2016 at 03:40:24PM -0400, Jeff Moyer wrote:
>> Christoph Hellwig  writes:
>> 
>> > On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
>> >> This ends up being a call to __sb_end_write:
>> >> 
>> >> void __sb_end_write(struct super_block *sb, int level)
>> >> {
>> >> percpu_up_read(sb->s_writers.rw_sem + level-1);
>> >> }
>> >> 
>> >> Nothing guarantees that submission and completion happen on the same
>> >> CPU.  Is this safe?
>> >
>> > Good point.  From my reading of the percpu_rwsem implementation it
>> > is not safe to release it from a different CPU.  Which makes me
>> > wonder how we can protect aio writes properly here..
>> 
>> Could we just change percpu_rw_semaphore->read_count to be a signed
>> integer?  The down_write path sums up the counters from all cpus...
>
> To what point?

Duh, nevermind.  You're right, it should work as-is.

-Jeff

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Peter Zijlstra]

On Mon, Oct 17, 2016 at 03:40:24PM -0400, Jeff Moyer wrote:
> Christoph Hellwig  writes:
> 
> > On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
> >> This ends up being a call to __sb_end_write:
> >> 
> >> void __sb_end_write(struct super_block *sb, int level)
> >> {
> >> percpu_up_read(sb->s_writers.rw_sem + level-1);
> >> }
> >> 
> >> Nothing guarantees that submission and completion happen on the same
> >> CPU.  Is this safe?
> >
> > Good point.  From my reading of the percpu_rwsem implementation it
> > is not safe to release it from a different CPU.  Which makes me
> > wonder how we can protect aio writes properly here..
> 
> Could we just change percpu_rw_semaphore->read_count to be a signed
> integer?  The down_write path sums up the counters from all cpus...

To what point?

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Peter Zijlstra]

On Mon, Oct 17, 2016 at 03:40:24PM -0400, Jeff Moyer wrote:
> Christoph Hellwig  writes:
> 
> > On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
> >> This ends up being a call to __sb_end_write:
> >> 
> >> void __sb_end_write(struct super_block *sb, int level)
> >> {
> >> percpu_up_read(sb->s_writers.rw_sem + level-1);
> >> }
> >> 
> >> Nothing guarantees that submission and completion happen on the same
> >> CPU.  Is this safe?
> >
> > Good point.  From my reading of the percpu_rwsem implementation it
> > is not safe to release it from a different CPU.  Which makes me
> > wonder how we can protect aio writes properly here..
> 
> Could we just change percpu_rw_semaphore->read_count to be a signed
> integer?  The down_write path sums up the counters from all cpus...

To what point?

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Peter Zijlstra]

On Mon, Oct 17, 2016 at 08:55:52PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
> > This ends up being a call to __sb_end_write:
> > 
> > void __sb_end_write(struct super_block *sb, int level)
> > {
> > percpu_up_read(sb->s_writers.rw_sem + level-1);
> > }
> > 
> > Nothing guarantees that submission and completion happen on the same
> > CPU.  Is this safe?
> 
> Good point.  From my reading of the percpu_rwsem implementation it
> is not safe to release it from a different CPU.  Which makes me
> wonder how we can protect aio writes properly here..

percpu-rwsem has the same semantics as regular rwsems, so preemptible
and 'owner' stuff.

Therefore we must support doing up from a different cpu than we did down
on; the owner could've been migrated while we held it.

And while there's a metric ton of tricky in the implementation, this
part is actually fairly straight forward. We only care about the direct
sum of the per-cpu counter, see readers_active_check() -> per_cpu_sum().

So one cpu doing an inc and another doing a dec summed is still 0.

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Peter Zijlstra]

On Mon, Oct 17, 2016 at 08:55:52PM +0200, Christoph Hellwig wrote:
> On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
> > This ends up being a call to __sb_end_write:
> > 
> > void __sb_end_write(struct super_block *sb, int level)
> > {
> > percpu_up_read(sb->s_writers.rw_sem + level-1);
> > }
> > 
> > Nothing guarantees that submission and completion happen on the same
> > CPU.  Is this safe?
> 
> Good point.  From my reading of the percpu_rwsem implementation it
> is not safe to release it from a different CPU.  Which makes me
> wonder how we can protect aio writes properly here..

percpu-rwsem has the same semantics as regular rwsems, so preemptible
and 'owner' stuff.

Therefore we must support doing up from a different cpu than we did down
on; the owner could've been migrated while we held it.

And while there's a metric ton of tricky in the implementation, this
part is actually fairly straight forward. We only care about the direct
sum of the per-cpu counter, see readers_active_check() -> per_cpu_sum().

So one cpu doing an inc and another doing a dec summed is still 0.

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Jeff Moyer]

Christoph Hellwig  writes:

> On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
>> This ends up being a call to __sb_end_write:
>> 
>> void __sb_end_write(struct super_block *sb, int level)
>> {
>> percpu_up_read(sb->s_writers.rw_sem + level-1);
>> }
>> 
>> Nothing guarantees that submission and completion happen on the same
>> CPU.  Is this safe?
>
> Good point.  From my reading of the percpu_rwsem implementation it
> is not safe to release it from a different CPU.  Which makes me
> wonder how we can protect aio writes properly here..

Could we just change percpu_rw_semaphore->read_count to be a signed
integer?  The down_write path sums up the counters from all cpus...

-Jeff

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Jeff Moyer]

Christoph Hellwig  writes:

> On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
>> This ends up being a call to __sb_end_write:
>> 
>> void __sb_end_write(struct super_block *sb, int level)
>> {
>> percpu_up_read(sb->s_writers.rw_sem + level-1);
>> }
>> 
>> Nothing guarantees that submission and completion happen on the same
>> CPU.  Is this safe?
>
> Good point.  From my reading of the percpu_rwsem implementation it
> is not safe to release it from a different CPU.  Which makes me
> wonder how we can protect aio writes properly here..

Could we just change percpu_rw_semaphore->read_count to be a signed
integer?  The down_write path sums up the counters from all cpus...

-Jeff

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Christoph Hellwig]

On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
> This ends up being a call to __sb_end_write:
> 
> void __sb_end_write(struct super_block *sb, int level)
> {
> percpu_up_read(sb->s_writers.rw_sem + level-1);
> }
> 
> Nothing guarantees that submission and completion happen on the same
> CPU.  Is this safe?

Good point.  From my reading of the percpu_rwsem implementation it
is not safe to release it from a different CPU.  Which makes me
wonder how we can protect aio writes properly here..

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Christoph Hellwig]

On Mon, Oct 17, 2016 at 02:19:47PM -0400, Jeff Moyer wrote:
> This ends up being a call to __sb_end_write:
> 
> void __sb_end_write(struct super_block *sb, int level)
> {
> percpu_up_read(sb->s_writers.rw_sem + level-1);
> }
> 
> Nothing guarantees that submission and completion happen on the same
> CPU.  Is this safe?

Good point.  From my reading of the percpu_rwsem implementation it
is not safe to release it from a different CPU.  Which makes me
wonder how we can protect aio writes properly here..

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Jeff Moyer]

Hi, Christoph,

Christoph Hellwig  writes:

> diff --git a/fs/aio.c b/fs/aio.c
> index 1157e13..bf315cd 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1078,6 +1078,17 @@ static void aio_complete(struct kiocb *kiocb, long 
> res, long res2)
>   unsigned tail, pos, head;
>   unsigned long   flags;
>  
> + if (kiocb->ki_flags & IOCB_WRITE) {
> + struct file *file = kiocb->ki_filp;
> +
> + /*
> +  * Tell lockdep we inherited freeze protection from submission
> +  * thread.
> +  */
> + __sb_writers_acquired(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> + file_end_write(file);

This ends up being a call to __sb_end_write:

void __sb_end_write(struct super_block *sb, int level)
{
percpu_up_read(sb->s_writers.rw_sem + level-1);
}

Nothing guarantees that submission and completion happen on the same
CPU.  Is this safe?

-Jeff

Re: [PATCH] aio: fix a use after free (and fix freeze protection of aio writes)

[Jeff Moyer]

Hi, Christoph,

Christoph Hellwig  writes:

> diff --git a/fs/aio.c b/fs/aio.c
> index 1157e13..bf315cd 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1078,6 +1078,17 @@ static void aio_complete(struct kiocb *kiocb, long 
> res, long res2)
>   unsigned tail, pos, head;
>   unsigned long   flags;
>  
> + if (kiocb->ki_flags & IOCB_WRITE) {
> + struct file *file = kiocb->ki_filp;
> +
> + /*
> +  * Tell lockdep we inherited freeze protection from submission
> +  * thread.
> +  */
> + __sb_writers_acquired(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> + file_end_write(file);

This ends up being a call to __sb_end_write:

void __sb_end_write(struct super_block *sb, int level)
{
percpu_up_read(sb->s_writers.rw_sem + level-1);
}

Nothing guarantees that submission and completion happen on the same
CPU.  Is this safe?

-Jeff