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 F1833C4332F for ; Wed, 14 Dec 2022 11:16:53 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8D4D18E0005; Wed, 14 Dec 2022 06:16:53 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 8849B8E0002; Wed, 14 Dec 2022 06:16:53 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7734A8E0005; Wed, 14 Dec 2022 06:16:53 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 678418E0002 for ; Wed, 14 Dec 2022 06:16:53 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 36C74A0EAD for ; Wed, 14 Dec 2022 11:16:53 +0000 (UTC) X-FDA: 80240659506.05.160B81D Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf08.hostedemail.com (Postfix) with ESMTP id BCD8A160012 for ; Wed, 14 Dec 2022 11:16:51 +0000 (UTC) Authentication-Results: imf08.hostedemail.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none); spf=pass (imf08.hostedemail.com: domain of cmarinas@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cmarinas@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1671016611; 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; bh=mLyCigGMJib5lH2noIwupm6EuLwKiM6G8aivd2LBAek=; b=Y+R0qDXkEVUbGp5kDdwRzModlU+ODJvGZISMZ8uUWDK9F5p9ktElZ6YUIKXa5eeJzPicHa gbExULoO4CfUrDgsb7XzBUHRjz8XFJhHxAvOZhCTpvJwnAhPfM7wc24ro2P6gZaAwW3XOg lf1AsK6QBexwbi2d5fQSZcPopkrA8Dw= ARC-Authentication-Results: i=1; imf08.hostedemail.com; dkim=none; dmarc=fail reason="SPF not aligned (relaxed), No valid DKIM" header.from=arm.com (policy=none); spf=pass (imf08.hostedemail.com: domain of cmarinas@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=cmarinas@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1671016611; a=rsa-sha256; cv=none; b=gE0ekjiFB20J0OynibQ9FsNNt4bcS3BJNOXhUvrZZgjsQJryGeiZmWolQ9Ixd95V+xgsJW SQjjT011qDjBaP0MJJnCXI3mScR5jIKolxjs+fVLv17icI0RicuwLffSj8XrCYRBh84105 od/lcCkIeVTM4BlEbqhUe8MmA5EEDhI= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id AF34B61998; Wed, 14 Dec 2022 11:16:50 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8E7BAC433EF; Wed, 14 Dec 2022 11:16:48 +0000 (UTC) Date: Wed, 14 Dec 2022 11:16:45 +0000 From: Catalin Marinas To: Waiman Long Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Muchun Song Subject: Re: [PATCH 2/2] mm/kmemleak: Fix UAF bug in kmemleak_scan() Message-ID: References: <20221210230048.2841047-1-longman@redhat.com> <20221210230048.2841047-3-longman@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221210230048.2841047-3-longman@redhat.com> X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: BCD8A160012 X-Stat-Signature: t88femypqkq7hcqeafsqejqy3dfhszin X-HE-Tag: 1671016611-881949 X-HE-Meta: U2FsdGVkX18YO/g/0eygOCX5yth+3FUV75WmICCywGiw2H1gCfhW3eCDb5Sa26T1bcTwO7F/0jOYiV+V5ymskA6dUKZMnwSPYKAWbCQ4MKF6T+4ucqJRDNw2NUVT7medgQdF0jdigwvZwUJP2ngnjwZqrTmWHQ10y+cgQnDQlv1GfPAxi4AUzOQPo5q2ZBNBgV7SremtKd/0bGaHuqvPE4/qMfswhI2XiZxTHcupgXVdWry+ebaw7yN+xzC9ZIqePl2QcfodRUf5DF5RmJ8EYQG4iJE/Q2C2UhaNQvTI4QYJXXUzfRutRKgK1ZUbgQ8oj6Sdo10lPB3VtUII7csLBIdCOYldgBwK/akVZuFqo6cwL72PEKU9heLZK1D78op5ElBBUa1ONnXKsZ8u19ACX+C7QOa76e2MgaU/PoDeuHUNB9wcngWNOqDXs4Gu7hWQ4o/+u2RwPr48uogLiDexNnak88SqVrhBaCXxo+4EnnTKz2jitlzVg9y68tMDusEUfC0Ik1r6ngzDXwCx6fgNwjIyoq9y+tEuG6DHI848C9+JND9scVQXs0Xd82yC0oH/E0SFnmMbg0gXjpZ9awtgBixsiYrl5G4675uI/EU5P7bnWOGyjiU3qBGB6Vm4SgVYzMR2VXJClbfKD1R0uqqXPkzOKS7IiGDigimEMkewwL6cVV2ZxgLZD6XFIXwdb0A5cacJdcGUP94SBedAn5mgNMvyRY6a7oWL0A5vN6KZ0hbX/LoKSyl28X3oYIRyyBsQoLOrfmpxhjsO5KPpAse5SqrccZsUT94EbQ1q9y4Ih27RYPGY0E0ZFLo5Q+gx76B6aWMyiPMhHJT5dB2X7JsPLMn90Rl+HvxjexJvsugn/L1AFnHQuaR7EVA7kWYLJfcKs3EqQOfzvtpVm05ooYES3vICANbB79g8MaGTI5UPFhtKIXHEwcMo66ODFZ5vpGEIiKQ2gNpO0b0rxNd1PNW aOb4IgBa nTbRAMg7DDeidp7qKlZu0WpeO/QiwjRkVBg9/ROPRAy10dgpvPT+3UbIMNRFf2O/pQx7vQwvhNq+yi6eI+uS6/4bt+7VfvrLpikFdfq4WfVMpRB5Pij+Cq5mnMjlPQ++98MQRDa6CqeqBUXs0KthrOcApJvSHg69/KeujgDUDm1OX2lK7RiC859kxPxNbs/gMOl9j9E/MODmXN4k= 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 Sat, Dec 10, 2022 at 06:00:48PM -0500, Waiman Long wrote: > Commit 6edda04ccc7c ("mm/kmemleak: prevent soft lockup in first > object iteration loop of kmemleak_scan()") fixes soft lockup problem > in kmemleak_scan() by periodically doing a cond_resched(). It does > take a reference of the current object before doing it. Unfortunately, > if the object has been deleted from the object_list, the next object > pointed to by its next pointer may no longer be valid after coming > back from cond_resched(). This can result in use-after-free and other > nasty problem. Ah, kmemleak_cond_resched() releases the rcu lock, so using list_for_each_entry_rcu() doesn't help. > diff --git a/mm/kmemleak.c b/mm/kmemleak.c > index 8c44f70ed457..d3a8fa4e3af3 100644 > --- a/mm/kmemleak.c > +++ b/mm/kmemleak.c > @@ -1465,15 +1465,26 @@ static void scan_gray_list(void) > * that the given object won't go away without RCU read lock by performing a > * get_object() if necessaary. > */ > -static void kmemleak_cond_resched(struct kmemleak_object *object) > +static void kmemleak_cond_resched(struct kmemleak_object **pobject) > { > - if (!get_object(object)) > + struct kmemleak_object *obj = *pobject; > + > + if (!(obj->flags & OBJECT_ALLOCATED) || !get_object(obj)) > return; /* Try next object */ I don't think we can rely on obj->flags without holding obj->lock. We do have a few WARN_ON() checks without the lock but in all other places the lock should be held. Another potential issue with re-scanning is that the loop may never complete if it always goes from the beginning. Yet another problem with restarting is that we may count references to an object multiple times and get more false negatives. I'd keep the OBJECT_ALLOCATED logic in the main kmemleak_scan() loop and retake the object->lock if cond_resched() was called (kmemleak_need_resched() returning true), check if it was freed and restart the loop. We could add a new OBJECT_SCANNED flag so that we skip such objects if we restarted the loop. The flag is reset during list preparation. I wonder whether we actually need the cond_resched() in the first loop. It does take a lot of locks but it doesn't scan the objects. I had a patch around to remove the fine-grained locking in favour of the big kmemleak_lock, it would make this loop faster (not sure what happened to that patch, I need to dig it out). -- Catalin