Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 6 Mar 2019 16:12:01 +0800
From:   Peter Xu <peterx@redhat.com>
To:     zhong jiang <zhongjiang@huawei.com>
Cc:     Mike Rapoport <rppt@linux.ibm.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        syzbot <syzbot+cbb52e396df3e565ab02@syzkaller.appspotmail.com>,
        Michal Hocko <mhocko@kernel.org>, cgroups@vger.kernel.org,
        Johannes Weiner <hannes@cmpxchg.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Linux-MM <linux-mm@kvack.org>,
        syzkaller-bugs <syzkaller-bugs@googlegroups.com>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        David Rientjes <rientjes@google.com>,
        Hugh Dickins <hughd@google.com>,
        Matthew Wilcox <willy@infradead.org>,
        Mel Gorman <mgorman@suse.de>, Vlastimil Babka <vbabka@suse.cz>,
        Mike Rapoport <rppt@linux.vnet.ibm.com>
Subject: Re: KASAN: use-after-free Read in get_mem_cgroup_from_mm
Message-ID: <20190306081201.GC11093@xz-x1>
References: <CACT4Y+Z+CH0UTdSz-w_woMPrBwg-GuobV1Su4qd9ReffTkyfVg@mail.gmail.com>
 <5C7D2F82.40907@huawei.com>
 <CACT4Y+agwaszODNGJHCqn4fSk4Le9exn3Cau0nornJ0RaTpDJw@mail.gmail.com>
 <5C7D4500.3070607@huawei.com>
 <CACT4Y+b6y_3gTpR8LvNREHOV0TP7jB=Zp1L03dzpaz_SaeESng@mail.gmail.com>
 <5C7E1A38.2060906@huawei.com>
 <20190306020540.GA23850@redhat.com>
 <5C7F6048.2050802@huawei.com>
 <20190306062625.GA3549@rapoport-lnx>
 <5C7F7992.7050806@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <5C7F7992.7050806@huawei.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 06, 2019 at 03:41:06PM +0800, zhong jiang wrote:
> On 2019/3/6 14:26, Mike Rapoport wrote:
> > Hi,
> >
> > On Wed, Mar 06, 2019 at 01:53:12PM +0800, zhong jiang wrote:
> >> On 2019/3/6 10:05, Andrea Arcangeli wrote:
> >>> Hello everyone,
> >>>
> >>> [ CC'ed Mike and Peter ]
> >>>
> >>> On Tue, Mar 05, 2019 at 02:42:00PM +0800, zhong jiang wrote:
> >>>> On 2019/3/5 14:26, Dmitry Vyukov wrote:
> >>>>> On Mon, Mar 4, 2019 at 4:32 PM zhong jiang <zhongjiang@huawei.com> wrote:
> >>>>>> On 2019/3/4 22:11, Dmitry Vyukov wrote:
> >>>>>>> On Mon, Mar 4, 2019 at 3:00 PM zhong jiang <zhongjiang@huawei.com> wrote:
> >>>>>>>> On 2019/3/4 15:40, Dmitry Vyukov wrote:
> >>>>>>>>> On Sun, Mar 3, 2019 at 5:19 PM zhong jiang <zhongjiang@huawei.com> wrote:
> >>>>>>>>>> Hi, guys
> >>>>>>>>>>
> >>>>>>>>>> I also hit the following issue. but it fails to reproduce the issue by the log.
> >>>>>>>>>>
> >>>>>>>>>> it seems to the case that we access the mm->owner and deference it will result in the UAF.
> >>>>>>>>>> But it should not be possible that we specify the incomplete process to be the mm->owner.
> >>>>>>>>>>
> >>>>>>>>>> Any thoughts?
> >>>>>>>>> FWIW syzbot was able to reproduce this with this reproducer.
> >>>>>>>>> This looks like a very subtle race (threaded reproducer that runs
> >>>>>>>>> repeatedly in multiple processes), so most likely we are looking for
> >>>>>>>>> something like few instructions inconsistency window.
> >>>>>>>>>
> >>>>>>>> I has a little doubtful about the instrustions inconsistency window.
> >>>>>>>>
> >>>>>>>> I guess that you mean some smb barriers should be taken into account.:-)
> >>>>>>>>
> >>>>>>>> Because IMO, It should not be the lock case to result in the issue.
> >>>>>>> Since the crash was triggered on x86 _most likley_ this is not a
> >>>>>>> missed barrier. What I meant is that one thread needs to executed some
> >>>>>>> code, while another thread is stopped within few instructions.
> >>>>>>>
> >>>>>>>
> >>>>>> It is weird and I can not find any relationship you had said with the issue.:-(
> >>>>>>
> >>>>>> Because It is the cause that mm->owner has been freed, whereas we still deference it.
> >>>>>>
> >>>>>> From the lastest freed task call trace, It fails to create process.
> >>>>>>
> >>>>>> Am I miss something or I misunderstand your meaning. Please correct me.
> >>>>> Your analysis looks correct. I am just saying that the root cause of
> >>>>> this use-after-free seems to be a race condition.
> >>>>>
> >>>>>
> >>>>>
> >>>> Yep, Indeed,  I can not figure out how the race works. I will dig up further.
> >>> Yes it's a race condition.
> >>>
> >>> We were aware about the non-cooperative fork userfaultfd feature
> >>> creating userfaultfd file descriptor that gets reported to the parent
> >>> uffd, despite they belong to mm created by failed forks.
> >>>
> >>> https://www.spinics.net/lists/linux-mm/msg136357.html
> >>>
> >> Hi, Andrea
> >>
> >> I still not clear why uffd ioctl can use the incomplete process as the mm->owner.
> >> and how to produce the race.
> > There is a C reproducer in  the syzcaller report:
> >
> > https://syzkaller.appspot.com/x/repro.c?x=172fa5a3400000
> >  
> >> From your above explainations,   My underdtanding is that the process handling do_exexve
> >> will have a temporary mm,  which will be used by the UUFD ioctl.
> > The race is between userfaultfd operation and fork() failure:
> >
> > forking thread                  | userfaultfd monitor thread
> > --------------------------------+-------------------------------
> > fork()                          |
> >   dup_mmap()                    |
> >     dup_userfaultfd()           |
> >     dup_userfaultfd_complete()  |
> >                                 |  read(UFFD_EVENT_FORK)
> >                                 |  uffdio_copy()
> >                                 |    mmget_not_zero()
> >     goto bad_fork_something     |
> >     ...                         |
> > bad_fork_free:                  |
> >       free_task()               |
> >                                 |  mem_cgroup_from_task()
> >                                 |       /* access stale mm->owner */
> >  
> Hi, Mike

Hi, Zhong,

> 
> forking thread fails to create the process ,and then free the allocated task struct.
> Other userfaultfd monitor thread should not access the stale mm->owner.
> 
> The parent process and child process do not share the mm struct.  Userfaultfd monitor thread's
> mm->owner should not point to the freed child task_struct.

IIUC the problem is that above mm (of the mm->owner) is the child
process's mm rather than the uffd monitor's.  When
dup_userfaultfd_complete() is called there will be a new userfaultfd
context sent to the uffd monitor thread which linked to the chlid
process's mm, and if the monitor thread do UFFDIO_COPY upon the newly
received userfaultfd it'll operate on that new mm too.

> 
> and due to the existence of tasklist_lock,  we can not specify the mm->owner to freed task_struct.
> 
> I miss something,=-O
> 
> Thanks,
> zhong jiang
> >> Thanks,
> >> zhong jiang
> 
> 

Regards,

-- 
Peter Xu