linux-kernel - Re: [PATCH] aio: fix mremap after fork null-deref

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: <x49ilhbl9qd.fsf@segfault.boston.devel.redhat.com>
Date:   Thu, 12 Jan 2023 16:32:42 -0500
From:   Jeff Moyer <jmoyer@...hat.com>
To:     Seth Jenkins <sethjenkins@...gle.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Benjamin LaHaise <bcrl@...ck.org>,
        linux-fsdevel@...r.kernel.org, linux-aio@...ck.org,
        linux-kernel@...r.kernel.org, Jann Horn <jannh@...gle.com>,
        Pavel Emelyanov <xemul@...allels.com>, stable@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH] aio: fix mremap after fork null-deref

Hi, Seth,

Seth Jenkins <sethjenkins@...gle.com> writes:

>> I wonder if it would be better to not copy the ring mapping on fork.
>> Something like the below?  I think that would be more in line with
>> expectations (the ring isn't available in the child process).
>
> I like this idea a lot, but would this be viewed as subtly changing
> the userland to kernel interface?

Yes, but...

The way things stand today, if you setup an io context in a process and
then fork a child, the child will be unable to use the aio system calls
to submit and reap I/Os.  This is because the ioctx_table was cleared
during fork, which means that lookup_ioctx() will not find the io
context.  However, the child still has access to the ring through the
memory mapping.  As a result, the child can reap I/O completions
directly from the ring.  That wasn't always the case.  The aio ring used
to be a MAP_PRIVATE mapping.  Commit 36bc08cc0170 ("fs/aio: Add support
to aio ring pages migration") changed it from a private to a shared
mapping, and I'm not sure why.  (That patch was included in v3.12, so
it's been this way for quite some time.)

With the patch I proposed (flagging the ring buffer with VM_DONTCOPY),
the child process would still be unable to submit and reap I/Os via the
aio system calls.  What changes is that the child process would now be
unable to reap completions via the shared ring buffer.  In fact, because
the ring is no longer mapped in the child process, any attempt to access
that memory would result in a segmentation fault.  However, I would be
very surprised if the interface was being used in this way.

> If we're okay with this change though, I think it makes sense.

My preference is to make the interface consistent.  I think setting
VM_DONTCOPY on the mapping is the right way forward.  I'd welcome other
opinions on whether the potential risk is worth it.

Cheers,
Jeff

>
>
> On Wed, Jan 11, 2023 at 2:37 PM Jeff Moyer <jmoyer@...hat.com> wrote:
>>
>> Hi, Seth,
>>
>> Seth Jenkins <sethjenkins@...gle.com> writes:
>>
>> > Commit e4a0d3e720e7 ("aio: Make it possible to remap aio ring") introduced
>> > a null-deref if mremap is called on an old aio mapping after fork as
>> > mm->ioctx_table will be set to NULL.
>> >
>> > Fixes: e4a0d3e720e7 ("aio: Make it possible to remap aio ring")
>> > Cc: stable@...r.kernel.org
>> > Signed-off-by: Seth Jenkins <sethjenkins@...gle.com>
>> > ---
>> >  fs/aio.c | 20 +++++++++++---------
>> >  1 file changed, 11 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/fs/aio.c b/fs/aio.c
>> > index 5b2ff20ad322..74eae7de7323 100644
>> > --- a/fs/aio.c
>> > +++ b/fs/aio.c
>> > @@ -361,16 +361,18 @@ static int aio_ring_mremap(struct vm_area_struct *vma)
>> >       spin_lock(&mm->ioctx_lock);
>> >       rcu_read_lock();
>> >       table = rcu_dereference(mm->ioctx_table);
>> > -     for (i = 0; i < table->nr; i++) {
>> > -             struct kioctx *ctx;
>> > -
>> > -             ctx = rcu_dereference(table->table[i]);
>> > -             if (ctx && ctx->aio_ring_file == file) {
>> > -                     if (!atomic_read(&ctx->dead)) {
>> > -                             ctx->user_id = ctx->mmap_base = vma->vm_start;
>> > -                             res = 0;
>> > +     if (table) {
>> > +             for (i = 0; i < table->nr; i++) {
>> > +                     struct kioctx *ctx;
>> > +
>> > +                     ctx = rcu_dereference(table->table[i]);
>> > +                     if (ctx && ctx->aio_ring_file == file) {
>> > +                             if (!atomic_read(&ctx->dead)) {
>> > +                                     ctx->user_id = ctx->mmap_base = vma->vm_start;
>> > +                                     res = 0;
>> > +                             }
>> > +                             break;
>> >                       }
>> > -                     break;
>> >               }
>> >       }
>>
>> I wonder if it would be better to not copy the ring mapping on fork.
>> Something like the below?  I think that would be more in line with
>> expectations (the ring isn't available in the child process).
>>
>> -Jeff
>>
>> diff --git a/fs/aio.c b/fs/aio.c
>> index 562916d85cba..dbf3b0749cb4 100644
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -390,7 +390,7 @@ static const struct vm_operations_struct aio_ring_vm_ops = {
>>
>>  static int aio_ring_mmap(struct file *file, struct vm_area_struct *vma)
>>  {
>> -       vma->vm_flags |= VM_DONTEXPAND;
>> +       vma->vm_flags |= VM_DONTEXPAND|VM_DONTCOPY;
>>         vma->vm_ops = &aio_ring_vm_ops;
>>         return 0;
>>  }
>>