From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id DF592C32771 for ; Wed, 28 Sep 2022 23:21:40 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1F3888D0002; Wed, 28 Sep 2022 19:21:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 1A2948D0001; Wed, 28 Sep 2022 19:21:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 06B558D0002; Wed, 28 Sep 2022 19:21:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id E8A498D0001 for ; Wed, 28 Sep 2022 19:21:39 -0400 (EDT) Received: from smtpin15.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id B92D4140929 for ; Wed, 28 Sep 2022 23:21:39 +0000 (UTC) X-FDA: 79963068318.15.F63C03D Received: from mail-yb1-f175.google.com (mail-yb1-f175.google.com [209.85.219.175]) by imf25.hostedemail.com (Postfix) with ESMTP id 63819A0004 for ; Wed, 28 Sep 2022 23:21:39 +0000 (UTC) Received: by mail-yb1-f175.google.com with SMTP id 65so10290155ybp.6 for ; Wed, 28 Sep 2022 16:21:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=QGQ9w7THnNEYoXoOGi0+JHDrSvkj2v2WI8525iyLAR0=; b=lT9XXApkYogaBy3s37CZRjJ3YE2yYFWSBV7F32ILRyI16jVmB9Vbbso+Y/d7RP3ZNJ JvUpXrS4MWf+6wl9afl637t0I1l396crUa1QqmpoBEAVmNs3paF0/4NIA70KFc9xXjaJ plzA3Si45eyzTWRO+DZZFMBq3CCc/MGGNOxv/l80yqXw1OIFBCMj1YUnfzq28qCekYZJ MjA+Qv04cjMjiZbfTKQhWYxvvnJGkqqiP3U4+gAaFmmda63Gi8giT2EOQB5ODlJ4ojzP 3vyKrN9gqqGZ2d2TIZxV9yyP4EHspKskhfZBAv7pnRsEhLSBuBOja5gP21FoVYx/+faP Dd3w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=QGQ9w7THnNEYoXoOGi0+JHDrSvkj2v2WI8525iyLAR0=; b=MohWZffjU8u7DTDUR6G8PL8gCbOtxLMhkkdxuyJCwoKjbh7Aqw43LlT6It1A5ZkCMI L8p6cVGaxm0eX3Dj6qbJ32toCwAq16HI4PsG9oB4Isk2KAaN3309DtO8j7Ewso0xC5MK n9za1Rl7Y2AdQJJfj8PazVKFmtWxTeL5i0nwQkk0Y29Hjx3S2K4ot+BrAm5xrjh8Omee cT4qqlulS4DiDUk7O7wbUw1D0Rm4FXOKJ2M4/JZFTW0tHfNs6N+46+YUIJiw9hKm9uuL kq5i/3yQAzO55huFfl9aOkiX2abv7xHh5RW6rc3EPwjzkzOgVWq+6czhpLrAmpfoVlWy AqLA== X-Gm-Message-State: ACrzQf0TxssJXnboxhE93jBuyTBR2lhtu/iMaVttXySsp5Vry6LVEsOJ okV7snS9KIo39+D+wsZfAvUfCz/mbs4xwfJK97LB9Q== X-Google-Smtp-Source: AMsMyM4JZPimpYhSiD8Slg5cQJvNfN4lSr71Pr+fLASDoB+xGMM6C89ByCc5Wpj5CYF1+ZR1c+ZR+A5uu3rkGFZbDJE= X-Received: by 2002:a25:4045:0:b0:6ae:b15a:cd81 with SMTP id n66-20020a254045000000b006aeb15acd81mr504245yba.340.1664407298432; Wed, 28 Sep 2022 16:21:38 -0700 (PDT) MIME-Version: 1.0 References: <20220913063625.3hgghufytudm6x4p@revolver> In-Reply-To: From: Suren Baghdasaryan Date: Wed, 28 Sep 2022 16:21:27 -0700 Message-ID: Subject: Re: BUG in binder_vma_close() at mmap_assert_locked() in stable v5.15 To: Carlos Llamas Cc: Liam Howlett , Andrew Morton , Michal Hocko , Guenter Roeck , Douglas Anderson , Christian Brauner , Greg Kroah-Hartman , "linux-mm@kvack.org" Content-Type: text/plain; charset="UTF-8" ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=lT9XXApk; spf=pass (imf25.hostedemail.com: domain of surenb@google.com designates 209.85.219.175 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1664407299; a=rsa-sha256; cv=none; b=pNsDQDTBB8AMBo85nASJiiPbYZrA7F/nAMoAb3Z9PRYu96v3Jx8J2r3AccbLxwzMdoS9uo MUoPTVLu1bLPX+CqtWjILKfpsRCIe0cMw3N8KzZrfjcFzakyIlWoDgWWeA5FpB5Pjqd1TK UxtbCXJgF2V0g9aqWC84/N7wfCai788= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1664407299; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=QGQ9w7THnNEYoXoOGi0+JHDrSvkj2v2WI8525iyLAR0=; b=JlnkFcnea30aAvBDKo5xExJQnKgQKAgJTTDdUiv87LGjVy2Z6jAR1B5pVZN+T6SVT5HjMn r6xy6XAGkD7NHZTOoehT9n4al1+gtQ5WiE+VewTxyvqz4wEW+63hKTnKY4Xjy6+CTGws4L 4I/fAjIHSEyUvRut0DSO5okjpoY5+1o= Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=lT9XXApk; spf=pass (imf25.hostedemail.com: domain of surenb@google.com designates 209.85.219.175 as permitted sender) smtp.mailfrom=surenb@google.com; dmarc=pass (policy=reject) header.from=google.com X-Stat-Signature: a756p3qnyuqjzgtyf17pn57kutkhds1m X-Rspamd-Queue-Id: 63819A0004 X-Rspam-User: X-Rspamd-Server: rspam11 X-HE-Tag: 1664407299-243635 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Sep 23, 2022 at 1:50 PM Carlos Llamas wrote: > > On Tue, Sep 13, 2022 at 06:36:33AM +0000, Liam Howlett wrote: > > > > It sounds like the binder_alloc vma_vm_mm is being used unsafely as > > well? I'd actually go the other way with this and try to add more > > validation that are optimized out on production builds. Since binder is > > saving a pointer to the mm struct and was saving the vma ponter, we > > should be very careful around how we use them. Is the mutex in > > binder_alloc protection enough for the vma binder buffers uses? How is > > the close() not being called before the exit_mmap() path? > > The alloc->mutex is top-level so it can't be used under vm_ops or we > risk a possible deadlock with mmap_lock unfortunately. > > > > > When you look at the mmget_not_zero() stuff, have a look at > > binder_alloc_new_buf_locked(). I think it is unsafely using the > > vma_vm_mm pointer without calling mmget_not_zero(), but the calling > > function is rather large so I'm not sure. > > We had used mm safely in places like binder_update_page_range() but not > so in the recent changes to switch over to vma_lookup(). It seems this > can be an issue if ->release() races with binder_alloc_print_allocated() > so I'll have a closer look at this. > > > So a fix for the initial BUG concern has landed in v5.15.70: > https://git.kernel.org/stable/c/899f4160b140 > > However, after doing a deeper investigation it seems there is still an > underlying problem. This requires a bit of context so please bear with > me while I try to explain. > > It started with the maple tree patchset in linux-next which added a > late allocation in mmap_region() in commit ("mm: start tracking VMAs > with maple tree"). Syzbot failed this allocation and so mmap_region() > unwinds, munmaps and frees the vma. This error path makes the cached > alloc->vma in binder a dangling pointer. > > Liam explains the scenario here: > https://lore.kernel.org/all/20220621020814.sjszxp3uz43rka62@revolver/ > > Also, Liam correctly points out that is safer to lookup the vma instead > of caching a pointer to it. Such change is what eventually is proposed > as the fix to the fuzzer report. > > However, I wonder why isn't ->mmap() being undone for this exit path in > mmap_region()? If anything fails _after_ call_mmap() it seems we > silently unmap and free the vma. What about allocations, atomic_incs, > and anything done inside ->mmap()? I think if ->mmap() fails it is expected to undo its own changes before returning the error. mmap_region() has no idea what kind of changes ->mmap() has done before it failed, therefore it can't undo them. At least that's how I understand it. > Shouldn't a ->close() be needed here > to undo these things as such: > -- > @@ -1872,6 +1889,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr, > > return addr; > > +undo_mmap: > + if (vma->vm_ops && vma->vm_ops->close) > + vma->vm_ops->close(vma); > unmap_and_free_vma: > fput(vma->vm_file); > vma->vm_file = NULL; > -- > I don't see mmap_region() calling vma->vm_ops->open() anywhere. So why would it have to call vma->vm_ops->close()? > I managed to reproduce the same syzbot issue in v5.15.41 by failing the > arch_validate_flags() check by simply passing PROT_MTE flag to mmap(). > I ran this in a "qemu-system-aarch64 -M virt,mte=on" system. > > Am I missing something? It looks scary to me all the memleaks, corrupt > ref counts, etc. that could follow from this simple path. > > -- > Carlos Llamas