Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 18 May 2023 10:40:52 -0400
From:   "Liam R. Howlett" <Liam.Howlett@Oracle.com>
To:     Carlos Llamas <cmllamas@google.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arve =?utf-8?B?SGrDuG5uZXbDpWc=?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>,
        Martijn Coenen <maco@android.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Christian Brauner <brauner@kernel.org>,
        Suren Baghdasaryan <surenb@google.com>,
        linux-kernel@vger.kernel.org, kernel-team@android.com,
        linux-mm@kvack.org
Subject: Re: [RFC PATCH 2/3] Revert "android: binder: stop saving a pointer
 to the VMA"
Message-ID: <20230518144052.xkj6vmddccq4v66b@revolver>
Mail-Followup-To: "Liam R. Howlett" <Liam.Howlett@Oracle.com>,
        Carlos Llamas <cmllamas@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arve =?utf-8?B?SGrDuG5uZXbDpWc=?= <arve@android.com>,
        Todd Kjos <tkjos@android.com>, Martijn Coenen <maco@android.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Christian Brauner <brauner@kernel.org>,
        Suren Baghdasaryan <surenb@google.com>,
        linux-kernel@vger.kernel.org, kernel-team@android.com,
        linux-mm@kvack.org
References: <20230424205548.1935192-1-cmllamas@google.com>
 <20230424205548.1935192-2-cmllamas@google.com>
 <20230424223419.6n2z72mocgmcj3aw@revolver>
 <ZEcMhOywwzsc6CN5@google.com>
 <20230425014328.d6vvimziv6je5xdg@revolver>
 <ZEmU8ULSEQhDVyX+@google.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZEmU8ULSEQhDVyX+@google.com>
User-Agent: NeoMutt/20220429
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?FJtRn//drIHPwExKA2Lcj41G1y2w6Xvq4AAlif5NIn0JTb5HxXQrbg9eMpRA?=
 =?us-ascii?Q?S8BLTOdFvPfd0qFcAKZ/iq2Z2ytrnaXaFo4+k/O0pLi/NrP7lKzsEI/oLcxp?=
 =?us-ascii?Q?IyDM1MkGHWWW4qX1XMglSWNoGbkGLHlNeO3SU0gftoSXZ1i/j2OKx10khQ6m?=
 =?us-ascii?Q?TLOFoRAL25BddzzwsPb+gf7y/3K+KPyvEQ6VCRLgahzgtvL+RKt0OHye/qRP?=
 =?us-ascii?Q?dVlW6hA/3Y3zcD0gzCPOSkXlv9/m+tudNgm0O4uJfp8RutGZUq1ZloE57y4l?=
 =?us-ascii?Q?Pap3daHolwhbeDnYBCDUq6Lwkk0v/P3qJdqbF7ZUSuBmZjoTT0FnjCezwMhj?=
 =?us-ascii?Q?HeCBtxXkXAbgD8jbngSnawtdZM6XgA+jUNXoPMJiIIFff9yKbrd47nBLqP1y?=
 =?us-ascii?Q?lGzJk/l8uUJhlPl81LgTeq1qAuEkqnBgXCVL4H8QJwq9HF/1TS4qqVvn3aU/?=
 =?us-ascii?Q?6uOaJ0gnCsc/8XgdEaHKbIlKhY1fjEvRDhzcIVxfvqEvrIqf5JyBa5zkh7Vh?=
 =?us-ascii?Q?1sxG8NLnxjgE+gr3I7pEGRh90XQFKW5jnUqZ7TEOnIcMZAW4j39YRIeVtZje?=
 =?us-ascii?Q?ew9l7CmNJiZszVXhS865+joENE5sbE0v3Q0YyaaOnwCuXhoDIOOYB/Xks/PZ?=
 =?us-ascii?Q?OrF34tUvzU17jKptgupcjpKkQ5Y2oMFfBxg1NvwJGhxrASmCmEcrlWobZeEl?=
 =?us-ascii?Q?VkJz+Uts64uo3jdvECs8ysEMbw8HNtX4OuV/8JzwHq98XauaxUT3ge6SjQcX?=
 =?us-ascii?Q?QTpNRFf0yvDdXMVfwKbJdz/o6l143Mn5EeFu0lZSCf98cF6G3hU9ohOgB8XJ?=
 =?us-ascii?Q?ZSiL/NXzgTDEdZxinbbSb/erFp6fMGtkz3xEtvpRULukWY1s/G0Or5PLHmiF?=
 =?us-ascii?Q?9iJXQJpFVpWUhCwyABDHElHgOHmGlAhKbR4NTDJFG2LTICq1Gk99qj8Rlpqd?=
 =?us-ascii?Q?37Vs6EycpoTYxUKvlUfBwrrWQU0XiiL/BQLIBt+etGMV4h5su40WHEPOA8DG?=
 =?us-ascii?Q?prVP54wO/69WUqW90HSmeAilzGrmKsdsc1qONtdm1ifNPo0eHIPexhS9hbXs?=
 =?us-ascii?Q?AMU9UK9+xkgYouu8gEMcTjDsZSD7PNkyKF4zRNm1wmKnrqFeXHzmNTrK6OlX?=
 =?us-ascii?Q?5C7OX+qdvSwvy0fWZpf9YpX6AvfbeKApSgzYcSJBCnEklpDysMQhwFXsErW+?=
 =?us-ascii?Q?xHgDWaXm0/1mLYygWhxpn7xReDvmUT6hGdpNXodxgzp7Mt4xqqQ/ZVKPFr1L?=
 =?us-ascii?Q?jPhNgrHm6TD3LZn75bOpKhXpNbmELDsuaf+Xm7fbVUUNWMsbiKuYzFo4HMgX?=
 =?us-ascii?Q?CERYOFcQl8XNdCJr4+o036+lCIvEepRBwU9TORcb5og2MUCKJTVvFm9nJEcX?=
 =?us-ascii?Q?wzMKXxGwpSSZlbp0rOAnz8QALgjDPy8NmW0LxhPq/RroVut/jzcMO6ZpagW5?=
 =?us-ascii?Q?z3wa9zf8Y3jjKDOUWi1ngp/AdlgebYQxnYjx6uVWebVLtU65NBaOppl0kmL/?=
 =?us-ascii?Q?do+7bZl7r6miWFUZm2NRjUPggYRc8rOemM3iQKXw93EQ7o/iIXdLoAqG51bb?=
 =?us-ascii?Q?pyknWU/ISSQaLHvJZzQTGNu9JjgGR7XI7vNoCb0R2bWh+/B5DQz5GkIp3imO?=
 =?us-ascii?Q?3A=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?XYALuExDJBI86YwzBvNaGemvWHXnn9anYj7R1Qhf+KWQ0AU3xiQV+3cg09NH?=
 =?us-ascii?Q?V1QcPr4Q/UpEtWdspRK1ISzHDFjfB8kPLYvW7J8WVANmAMamN7A1srqvmbkL?=
 =?us-ascii?Q?BviKAoYyedWTsr2L9Yxw+dw1E+DFZJQwiE3S4gu28Nw1rTDAEu0UqpP4MDWP?=
 =?us-ascii?Q?Wqfqc5drMVhvWsunF1+k07lI//AU/DLw/b2pojgx4aI08tvyhan+LI5Ts1N5?=
 =?us-ascii?Q?1nqduZydFMEALSeOwUS+lCEJqDqlz1cEf6TEzTFpSj5twz0PR9Coby/2AlT9?=
 =?us-ascii?Q?ODAgqNc6marGGEG5RDRH2tv4N47mUAxzwcC62iYxZ+pDNVkZHP9EdNkppSC3?=
 =?us-ascii?Q?DJFlVPiMFl7tuUo275xGJ7WQenIBgVMXtUWMRBu8AEgvODk47eIRJQwAMjmj?=
 =?us-ascii?Q?nrhoImKPuzhpLAshlw21VhGNUAihpTOIJVlgz/zNDT/KGNpo7ySYv4GR7+9I?=
 =?us-ascii?Q?8V9PfrlmXzJWYvZSG0zgYmuM0hkBbiIU4G5/80wUgue/Ig+FywBT9T9qYa8X?=
 =?us-ascii?Q?paZl7HJXHGqcbmdSW7H+veOUURd1FSTH11GQZVvpFqdAOM3LQWektEUwObQ0?=
 =?us-ascii?Q?xt43zcjxccruDsv5ncXvb1YBXTgm3WkRJ37thluO7b4oBy/mcPAOyByDg7y2?=
 =?us-ascii?Q?7AUb2+/XW8IEVMWgaCNMsYxONAqqfMizoas7vIKPyAc3MTywkdQRrmYuDK8P?=
 =?us-ascii?Q?EYeXDswrv8PDFsG4RKe3RXk8n+Xm1xaMYnn8Kw33tizYZgmOVe4ToKrv1N1v?=
 =?us-ascii?Q?5SHlLSoTUA0h67dI1QN7k11hAhrewW5ZLUYO4WI8o4ffJTIG8wYjdBI91t2+?=
 =?us-ascii?Q?kBtU0kGoepwL8BGICgBnoIWgiBAiYQBBUv+G4aydiw0LT4So4m9EeB/36XLI?=
 =?us-ascii?Q?rhjqJfYqi+sAiHIK2laA2n8ccYJKochFWFD3Ax+ErOdYThSGQ6hQOlNknwhv?=
 =?us-ascii?Q?9PkOWNox83MPzDV8H+msc1ipGp7Tbw/h3xT9HPemyrE=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 78388b83-7f2f-4655-cd32-08db57ade376
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 May 2023 14:40:56.1544
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qcCc7LWCdD68vlHEM3EiHcqr6W9qthWjyGKUqPb7Ys/lYJwegqQ8hF2Y/YLe+tFmJ6o3NBOlZYzNyQ4UB3ZL3Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4630
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-18_11,2023-05-17_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=928 adultscore=0
 malwarescore=0 mlxscore=0 spamscore=0 bulkscore=0 phishscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2304280000 definitions=main-2305180118
X-Proofpoint-GUID: TxxDJpqc7HY10c78et6MpUgqQXcZq0_C
X-Proofpoint-ORIG-GUID: TxxDJpqc7HY10c78et6MpUgqQXcZq0_C
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* Carlos Llamas <cmllamas@google.com> [230426 17:17]:
> On Mon, Apr 24, 2023 at 09:43:28PM -0400, Liam R. Howlett wrote:
> > * Carlos Llamas <cmllamas@google.com> [230424 19:11]:
> > > 
> > > The specifics are in the third patch of this patchset but the gist of it
> > > is that during ->mmap() handler, binder will complete the initialization
> > > of the binder_alloc structure. With the last step of this process being
> > > the caching of the vma pointer. Since the ordering is protected with a
> > > barrier we can then check alloc->vma to determine if the initialization
> > > has been completed.
> > > 
> > > Since this check is part of the critical path for every single binder
> > > transaction, the performance plummeted when we started contending for
> > > the mmap_lock. In this particular case, binder doesn't actually use the
> > > vma.
> > 
> > So why does binder_update_page_range() take the mmap_read_lock then use
> > the cached vma in the reverted patch?
> > 
> > If you want to use it as a flag to see if the driver is initialized, why
> > not use the cached address != 0?
> > 
> > Or better yet,
> > 
> > >It only needs to know if the internal structure has been fully
> > > initialized and it is safe to use it.
> > 
> > This seems like a good reason to use your own rwsem.  This is,
> > essentially, rolling your own lock with
> > smp_store_release()/smp_load_acquire() and a pointer which should not be
> > cached.
> 
> We can't use an rwsem to protect the initialization. We already have an
> alloc->mutex which would be an option. However, using it under ->mmap()
> would only lead to dead-locks with the mmap_lock.
> 
> I agree with you that we could use some other flag instead of the vma
> pointer to signal the initialization. I've actually tried several times
> to come up with a scenario in which caching the vma pointer becomes an
> issue to stop doing this altogether. However, I can't find anything
> concrete.
> 
> I don't think the current solution in which we do all these unnecessary
> vma lookups is correct. Instead, I'm currently working on a redesign of
> this section in which binder stops to allocate/insert pages manually. We
> should be making use of the page-fault handler and let the infra handle
> all the work. The overall idea is here:
> https://lore.kernel.org/all/ZEGh4mliGHvyWIvo@google.com/
> 
> It's hard to make the case for just dropping the vma pointer after ~15
> years and take the performance hit without having an actual issue to
> support this idea. So I'll revert this for now and keep working on the
> page-fault solution.
> 

I came across this [1] when I was looking into something else and
thought I'd double back and make sure your fix for this UAF is also
included, since your revert will restore this bug.

I do still see the mmap_read_lock() in binder_update_page_range() vs the
required mmap_write_lock(), at least in my branch.

[1] https://lore.kernel.org/all/20221104175450.306810-1-cmllamas@google.com/

Thanks,
Liam