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 CFABBC07E9D for ; Fri, 23 Sep 2022 20:50:36 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 6E6AD80022; Fri, 23 Sep 2022 16:50:36 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6956080016; Fri, 23 Sep 2022 16:50:36 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 583F880022; Fri, 23 Sep 2022 16:50:36 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 49E6980016 for ; Fri, 23 Sep 2022 16:50:36 -0400 (EDT) Received: from smtpin27.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 24C99A06F3 for ; Fri, 23 Sep 2022 20:50:36 +0000 (UTC) X-FDA: 79944543672.27.B834C0C Received: from mail-pg1-f169.google.com (mail-pg1-f169.google.com [209.85.215.169]) by imf23.hostedemail.com (Postfix) with ESMTP id C9E9114001C for ; Fri, 23 Sep 2022 20:50:35 +0000 (UTC) Received: by mail-pg1-f169.google.com with SMTP id 78so1267760pgb.13 for ; Fri, 23 Sep 2022 13:50:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date; bh=UDz0m44NMwWnYrDB6Stx+rrTQqUwIjOfoA+2/H6V/LE=; b=HsakbeiPWaZSBXQ1dmr0+EGAzU4kIDAOnVFIlwzAEn/CxfUNP2LImo0HH+Ne/qKd08 5U1ragtqU4p1qhAx3I0eEtd5DjNiWC4LSscyzQAFA+SPgPdF4gXXYpXMurRxytrhfUMv P5Xgm1LSXFwRoVZNuA2sjLZp3In5e9zNSP3przKM0ZqveYq4wIuWKi7oP7mGG7YVjIH+ 1nBW/zv3LHagyAVUxSDg+W5kBSj59HXuMqMChU1Wq1DbB8pxPPWK6hWW0ztjtnzluRp9 4UHg6KvSdwFhkVkRrR47ETjSKzzaEZZ9GRGvg7toP/anuoZOmPCSUO8VHLPFQkSd6Gph u5Ig== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date; bh=UDz0m44NMwWnYrDB6Stx+rrTQqUwIjOfoA+2/H6V/LE=; b=tHZ5QmVvVoUjEOZj91RsVwcSyCnY1ZAbP6OMhI5p9sVajnyWxpfgcF4Ow6n5VD/ODJ 1Thh3r1ibZz9UH4rZF1nQJqIW6Vi/mxeR5BoXOKr3bhuE2aEudNHjFwTfuwI+VpVI2Y/ HrnZZm3elQ3NgN6b2xlChexmllrOggyfb2twL2ZZ6GGwByoyHevJ04L4K/1PcuSaHsNu 9CjFyCFa2OjWVMnlTD+34LfTJl6XG8yYD+m1154a1XWvf9QfCr4DuXhXBvNzrx0Vccab g43Rqw+FUGibzJxNE0cvcou3j4vxDzfrDQaToOSDgiqF1oCRuXIt5WvgROnuRMccJfgy Au6w== X-Gm-Message-State: ACrzQf0Sbb/HHmsLxAF+3t4x9/xZT9BQ6X/LyLeAAAPvFcuuLQB4PiHS UbpiLHFcRfcL2kIzTM+4OOrdrg== X-Google-Smtp-Source: AMsMyM4uaGWuErnpQwjjtbbnOj7ZUh7J3+/n6U807qnT68u2GpmDq/k+YVOWugU+lnMr1L2dz4D4hQ== X-Received: by 2002:aa7:97b4:0:b0:547:f861:1fc3 with SMTP id d20-20020aa797b4000000b00547f8611fc3mr11101313pfq.42.1663966234605; Fri, 23 Sep 2022 13:50:34 -0700 (PDT) Received: from google.com (157.214.185.35.bc.googleusercontent.com. [35.185.214.157]) by smtp.gmail.com with ESMTPSA id v28-20020aa799dc000000b0053e4baecc14sm6990651pfi.108.2022.09.23.13.50.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 23 Sep 2022 13:50:34 -0700 (PDT) Date: Fri, 23 Sep 2022 20:50:29 +0000 From: Carlos Llamas To: Liam Howlett Cc: Suren Baghdasaryan , Andrew Morton , Michal Hocko , Guenter Roeck , Douglas Anderson , Christian Brauner , Greg Kroah-Hartman , "linux-mm@kvack.org" Subject: Re: BUG in binder_vma_close() at mmap_assert_locked() in stable v5.15 Message-ID: References: <20220913063625.3hgghufytudm6x4p@revolver> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220913063625.3hgghufytudm6x4p@revolver> ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1663966235; a=rsa-sha256; cv=none; b=edXLk9LgyQCdpviE5ud8E/5J3UluQ/EdmRXdPlcYrko+JsJ4QYqrwHGi92AvCn5iF3POjf yLf2HxbdFw1HN5fmGfcfrXgf5iRsCqHhNVEy+fuGr53qLXrzLzNl/aCHcNrz1rHmn2+XkW 6V9GHGPt3PTbOtkZf3cjMLvXAW1HNKg= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=HsakbeiP; spf=pass (imf23.hostedemail.com: domain of cmllamas@google.com designates 209.85.215.169 as permitted sender) smtp.mailfrom=cmllamas@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1663966235; 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=UDz0m44NMwWnYrDB6Stx+rrTQqUwIjOfoA+2/H6V/LE=; b=E23n+TsapVbaLFQLyP4XNb4yKayU6ro0nbYYZGJ5JqlRK/P7+/tG337yQ2xxgIi4cEDl3L IBI2FrtG68z8IseIXt+HevCEzbZSiFEUEG91jlUBbRk7k+V8xiw+cAsjMBjyCbi51hvPDA nL7xkIy4kDLroJwHH3fpFZYglZTWKbM= X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: C9E9114001C Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=HsakbeiP; spf=pass (imf23.hostedemail.com: domain of cmllamas@google.com designates 209.85.215.169 as permitted sender) smtp.mailfrom=cmllamas@google.com; dmarc=pass (policy=reject) header.from=google.com X-Rspam-User: X-Stat-Signature: thqhortzsx4b1te3n14rrhq1dhmjd5ir X-HE-Tag: 1663966235-692065 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 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()? 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 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