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 X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, MSGID_FROM_MTA_HEADER,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5F781C433E4 for ; Mon, 27 Jul 2020 13:27:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 31C6B2075A for ; Mon, 27 Jul 2020 13:27:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=amdcloud.onmicrosoft.com header.i=@amdcloud.onmicrosoft.com header.b="Ni3dESiy" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728803AbgG0N1E (ORCPT ); Mon, 27 Jul 2020 09:27:04 -0400 Received: from mail-dm6nam11on2042.outbound.protection.outlook.com ([40.107.223.42]:56192 "EHLO NAM11-DM6-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726139AbgG0N1D (ORCPT ); Mon, 27 Jul 2020 09:27:03 -0400 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=kFAHTGOvU/Df27D5RbKQ6YbkJODhNcipykVRaRpmlatETLodXrQVE+Kquid8Eo9IkjYYB76zUbPHn0UKalhXNQ+6po8VPfn6SUgWpJsG/vh6XG8aJTAXxgKLnIvkC3hbkzPklkCcXtLSphN/SXolH7Lk+aHzO2xude9WHdYyVHZARpUciJKIO6P18BW409/yxMlJMlbGMUZK5YD81TzDgFtCmYglMiRs3XV+50NnOjAeSpoiJLTVRnGOMMK3bk2tmaN6coqwXWkATw50ILXaPcIP7fW1ssnOl2dEXE6kr8Ck5/0Uj5fVx+lSyuddJ16wvgWfVQNhKqx9svXRYO/Rog== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=G/nkM/kKJ2ZuwPdrY8KfAKFJMCz4i9EFVFcZ5w0rD6g=; b=MTZeN8Jv4UG5iPPKazhh8ulu9fhmpTzdNKxkYu/QpRmlIXXpTH9q+KTl9yswR7hHeGX9nR0vvUh8bCbd6LoIPYPkffGoohnoxQrGAVr2sn/0S8heLUyi5Vrw8yKE+EDJNLP9+QC+PkFtL4hCixXY7LhJKYMo+//m146jg4juRZV5wbM8o+VQsKb/+SLVbo+4ghY89za8XCIg8WqHj7tAN0Lhax29maBmqCC2XvBxuKDPJkkcx4nOonySuqu1HkL1CweGxNlTz4GNT9MZ30jLttCdjDVTiik7Y6C3/yCzepApcB0uUnLvL0kEzbiH/+3p0TBk6G7Lv5wtrXLTY+odsQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amdcloud.onmicrosoft.com; s=selector2-amdcloud-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=G/nkM/kKJ2ZuwPdrY8KfAKFJMCz4i9EFVFcZ5w0rD6g=; b=Ni3dESiyDWtQYANIj8yi4JR/4tLqwFDyPGhe2mK7Lwbr2NKeWWuODM4zs7V1HclCn2YrDt4IATwJP38tNM207RMFEOQMaFM0P+bpYanDaH4Zb8RMeFuhjujj0bYaT5YWc5JYGFH+LbErnG0+KgOkEIuqcu8AuonBtdYgoU8Xdiw= Authentication-Results: amd.com; dkim=none (message not signed) header.d=none;amd.com; dmarc=none action=none header.from=amd.com; Received: from DM6PR12MB3561.namprd12.prod.outlook.com (2603:10b6:5:3e::26) by DM6PR12MB4090.namprd12.prod.outlook.com (2603:10b6:5:217::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3216.23; Mon, 27 Jul 2020 13:26:59 +0000 Received: from DM6PR12MB3561.namprd12.prod.outlook.com ([fe80::6df0:61cb:d37:2a0b]) by DM6PR12MB3561.namprd12.prod.outlook.com ([fe80::6df0:61cb:d37:2a0b%6]) with mapi id 15.20.3216.033; Mon, 27 Jul 2020 13:26:59 +0000 Subject: Re: [PATCH] drm/amd/display: Clear dm_state for fast updates To: Mazin Rezk , "linux-kernel@vger.kernel.org" , "amd-gfx@lists.freedesktop.org" , "dri-devel@lists.freedesktop.org" Cc: Paul Menzel , "anthony.ruhier@gmail.com" , Duncan <1i5t5.duncan@cox.net>, Kees Cook , "sunpeng.li@amd.com" , "regressions@leemhuis.info" , Alexander Deucher , Andrew Morton , "mphantomx@yahoo.com.br" , =?UTF-8?Q?Christian_K=c3=b6nig?= References: From: "Kazlauskas, Nicholas" Message-ID: Date: Mon, 27 Jul 2020 09:26:54 -0400 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-ClientProxiedBy: YQXPR01CA0106.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:c00:41::35) To DM6PR12MB3561.namprd12.prod.outlook.com (2603:10b6:5:3e::26) MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.1.180] (24.212.165.133) by YQXPR01CA0106.CANPRD01.PROD.OUTLOOK.COM (2603:10b6:c00:41::35) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3216.23 via Frontend Transport; Mon, 27 Jul 2020 13:26:57 +0000 X-Originating-IP: [24.212.165.133] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: 02e6dd5c-9f22-4225-db51-08d83230bd31 X-MS-TrafficTypeDiagnostic: DM6PR12MB4090: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:1751; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: PjpkNFXzsHZ31Ry2g+HAH7K+oosCbZJObYRx7n9bf69WM+mXDVcuXEGowSU5Wlce3gy4gHmSN+DaQLwlodidS/iIEx7K4L71fO1KKwdg707JJ9J6cwBvO9JCKBAhiWeBSGJPMeDZqyuJRjDL8GCFC3YL/H76dU2wjrzXeqlMdUJW+Qa9xUDhA6jAjuxwdmrHSNHqKV8O+nSUdTmb5xtiPBbUFocji+hB6Q3++i24cAaHm4Z7TQl+aJuRi6SYflhACyxkrnAr4q22F5tTEwyhG924Pv6m0iWawUvl8Mogbjgl4nLQi73ShQVO/yZWYWXeQqSWKudvOph8PA0GYeSvTJe6EK7DGTwmgTD3yvGNHMQ7UYbypi2mzUSlZXWMq381FobG0DsffsH2ipi7RvBv25LRj1BhdCAGaWY2JwsaOqVYXPaoxssq6d9IECbfsKksf+2JUZkAvs3CKkcvd0hxfg== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM6PR12MB3561.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(376002)(396003)(136003)(366004)(346002)(39860400002)(53546011)(16526019)(66556008)(31696002)(966005)(316002)(36756003)(6666004)(478600001)(31686004)(4326008)(6486002)(26005)(16576012)(15650500001)(86362001)(66946007)(110136005)(2906002)(956004)(66476007)(8676002)(52116002)(2616005)(54906003)(186003)(5660300002)(83380400001)(7416002)(8936002)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: R5wrgS1ARCTm50a/ei1MLMxU+BR8p1qdckjS8rpthBn03Of0kOYVzBlCNJy5XULwWXtfjmIABWZEd5+eDiPVz71J+uX4X9xW9ESBBMYG0yeSEzke9Htn6QkWQz6afj5HT+QMnQkl1EQyJ0fbLy122qL6aQLrH8xfIs0RO53tajWVUfzQtNfhBIvDfu7tVOLF2lbqBio2Bft/o9DA14bFOw8+2Mx0GsmSRJzGVcjoUYypUYErx+lbA4RQ4fFsiAZlqUhd9ZqJMCog+qpew3hkrjHv3nISYh31ORGojSdE1s3JkJWIqFZfxR8e4vcxFkouB55btb5aAzjS3U2PglS+zD/8PdnrtblxG4Iy3SfvdnV2sziNGTRQYrCDhD7u7WQTbCV5h9p7r9dxij8SsDNxTtc41t7aWKx067SPBjkV7vT36kUgfoi5XNezcFnAaMQtWjwHdZqbzsvawEi/pTXMilsvhaKX5W0wvXEp66IAWhE1+fPnnxENlQgrrtH20Wtv X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 02e6dd5c-9f22-4225-db51-08d83230bd31 X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3561.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jul 2020 13:26:58.9394 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: B9mV7BL7G/ifAgZEOqI3nJ3q/Pyx7d8wulJMvrdPL4fpx/CPMUWNsjhNS5Z9w40BDy58hWQnnQw8fkzv7t23tQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4090 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2020-07-27 1:40 a.m., Mazin Rezk wrote: > This patch fixes a race condition that causes a use-after-free during > amdgpu_dm_atomic_commit_tail. This can occur when 2 non-blocking commits > are requested and the second one finishes before the first. Essentially, > this bug occurs when the following sequence of events happens: > > 1. Non-blocking commit #1 is requested w/ a new dm_state #1 and is > deferred to the workqueue. > > 2. Non-blocking commit #2 is requested w/ a new dm_state #2 and is > deferred to the workqueue. > > 3. Commit #2 starts before commit #1, dm_state #1 is used in the > commit_tail and commit #2 completes, freeing dm_state #1. > > 4. Commit #1 starts after commit #2 completes, uses the freed dm_state > 1 and dereferences a freelist pointer while setting the context. > > Since this bug has only been spotted with fast commits, this patch fixes > the bug by clearing the dm_state instead of using the old dc_state for > fast updates. In addition, since dm_state is only used for its dc_state > and amdgpu_dm_atomic_commit_tail will retain the dc_state if none is found, > removing the dm_state should not have any consequences in fast updates. > > This use-after-free bug has existed for a while now, but only caused a > noticeable issue starting from 5.7-rc1 due to 3202fa62f ("slub: relocate > freelist pointer to middle of object") moving the freelist pointer from > dm_state->base (which was unused) to dm_state->context (which is > dereferenced). > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=207383 > Fixes: bd200d190f45 ("drm/amd/display: Don't replace the dc_state for fast updates") > Reported-by: Duncan <1i5t5.duncan@cox.net> > Signed-off-by: Mazin Rezk > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36 ++++++++++++++----- > 1 file changed, 27 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 86ffa0c2880f..710edc70e37e 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > @@ -8717,20 +8717,38 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, > * the same resource. If we have a new DC context as part of > * the DM atomic state from validation we need to free it and > * retain the existing one instead. > + * > + * Furthermore, since the DM atomic state only contains the DC > + * context and can safely be annulled, we can free the state > + * and clear the associated private object now to free > + * some memory and avoid a possible use-after-free later. > */ > - struct dm_atomic_state *new_dm_state, *old_dm_state; > > - new_dm_state = dm_atomic_get_new_state(state); > - old_dm_state = dm_atomic_get_old_state(state); > + for (i = 0; i < state->num_private_objs; i++) { > + struct drm_private_obj *obj = state->private_objs[i].ptr; > > - if (new_dm_state && old_dm_state) { > - if (new_dm_state->context) > - dc_release_state(new_dm_state->context); > + if (obj->funcs == adev->dm.atomic_obj.funcs) { > + int j = state->num_private_objs-1; > > - new_dm_state->context = old_dm_state->context; > + dm_atomic_destroy_state(obj, > + state->private_objs[i].state); > + > + /* If i is not at the end of the array then the > + * last element needs to be moved to where i was > + * before the array can safely be truncated. > + */ > + if (i != j) > + state->private_objs[i] = > + state->private_objs[j]; > > - if (old_dm_state->context) > - dc_retain_state(old_dm_state->context); > + state->private_objs[j].ptr = NULL; > + state->private_objs[j].state = NULL; > + state->private_objs[j].old_state = NULL; > + state->private_objs[j].new_state = NULL; > + > + state->num_private_objs = j; > + break; > + } In the bug report itself I mentioned that I don't really like hacking around the DRM core for resolving this patch but to go into more specifics, it's really two issues of code maintenance: 1. It's iterating over internal structures and layout of private objects in the state and modifying the state. The core doesn't really guarantee how these things are going to be laid out and it may change in the future. 2. It's freeing an allocation we don't own from DM. DRM doesn't track this state elsewhere for purposes of freeing, but nothing is really stopping the core from doing this later down the line. The implementation itself is correct from a technical perspective, but I'd rather it reside in DRM as a helper for code maintenance purposes. Regards, Nicholas Kazlauskas > } > } > > -- > 2.27.0 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >