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]

Date:   Fri, 13 Jan 2023 11:06:31 -0500
From:   Jeff Moyer <jmoyer@...hat.com>
To:     Benjamin LaHaise <bcrl@...ck.org>
Cc:     Seth Jenkins <sethjenkins@...gle.com>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        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, Ben,

Thanks for taking the time to chime in.

Benjamin LaHaise <bcrl@...ck.org> writes:

> On Thu, Jan 12, 2023 at 04:32:42PM -0500, Jeff Moyer wrote:
>> 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.
>
> VM_DONTCOPY makes sense, but a SEGV is a pretty bad failure mode.  Any
> process reaping events in the child after fork() isn't going to be
> consistent in behaviour, and is able to see partial completion of an I/O
> and other inconsistencies, so they're going to be subtly broken at best.
>
> Unfortunately, we have no way of knowing if this behaviour is exercised
> anywhere without changing it and waiting for someone to holler.

OK, so it sounds like you would rather err on the side of caution.
That's fine with me.

Seth, I'll go back and review your patch.

Thanks!
Jeff