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:   Tue, 28 Mar 2023 17:48:15 -0700
From:   Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
To:     Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
CC:     Min Li <lm0963hack@gmail.com>, <jani.nikula@linux.intel.com>,
        "Lionel Landwerlin" <lionel.g.landwerlin@intel.com>,
        <joonas.lahtinen@linux.intel.com>, <rodrigo.vivi@intel.com>,
        <airlied@gmail.com>, <daniel@ffwll.ch>,
        <intel-gfx@lists.freedesktop.org>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/1] drm/i915: fix race condition UAF in
 i915_perf_add_config_ioctl
Message-ID: <ZCOKz62qE7jASyg1@orsosgc001.jf.intel.com>
References: <20230328093627.5067-1-lm0963hack@gmail.com>
 <e7541f73-f100-3b1f-de80-376fa55f2479@linux.intel.com>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Disposition: inline
In-Reply-To: <e7541f73-f100-3b1f-de80-376fa55f2479@linux.intel.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?M0tWcHNCNFBWVldvUkpvQkF1Um83Yk5BNXBjMmx4bkd4dlh2SzVmL2sya2NP?=
 =?utf-8?B?TVh0UDZNR2lXSVh3b1BWc1pHeGJVaDdlTWNOdnR4ajBiTDNISkxlQ21BM2xY?=
 =?utf-8?B?SXRjOEl2WUVNVEFSK0RVOEVrLzlmanRTWUtqR1lWdE9pQXMyb1JTbFBHeUdU?=
 =?utf-8?B?bkxEZzV1OGl2MmxPWUdvMjVJRGg2NnNKSTl1OFd4bURZbDVTcGxDbmZuR0ZD?=
 =?utf-8?B?Q2YvUHZZd0VMMk9iV05jYmVzblZabVUyYWo1K01jdUd3SThUeTUrb0J2a0xw?=
 =?utf-8?B?Y1lKNVpvSU00dzNYdFQ3dHYyaDhicFcrTU5kMk5NaytNRCs5MTZrckJLc3I3?=
 =?utf-8?B?UXgrZzZyR28rSWpOc09nTFdISGIyejZ3MGMrY1dRRHpFT3NSaXltT2lWRnly?=
 =?utf-8?B?WkFxVDJtUFdRS3ZHK1Z4bWJ2Ni9XYWZCMWZ0Z1BLb2tUenhvRFJiZnRMamtl?=
 =?utf-8?B?SmhLMTlhclJDbXVEZEgxaWlpbkhSRStwKzQxRHM5dnJhMGVsZUxuaklKbUl4?=
 =?utf-8?B?R2V6SDFsMkpXN0RHenJLR2Q2Z1Q5bjVKVDNxTUFlQTJpeVpVSytpZHhWOWw0?=
 =?utf-8?B?dzhxZkZ4QmF6QUU4Nk5jN3VVSXJVWmtMNHBvQjBoQUZ1VlNpQi8wKzVYQlBs?=
 =?utf-8?B?a1pzdUpxZGpENGZRMHByK3dwSTIxdkVqUkFINGJIWVN5ZERTdXlrVS85Qkxk?=
 =?utf-8?B?UGdBZTJITWZqSDBKcFVGMGM1OXJIRkZWTkZVcWJWUVl1S2JlWGpmMFBLNElB?=
 =?utf-8?B?THk1R21QaFRiTHhqSy80UkVqN1FJeVh3elNyVFJFbGZPTzRYWTJRSFZ2K00v?=
 =?utf-8?B?UlR1Ym4wcnBVcWNuRlhUREdMQ0VvanpicndpYVNWQ0JjRXVzQVFzelRlNXJZ?=
 =?utf-8?B?Q3prZWdkSDJuTWczVmZJeDc2Nm1ZMzBxTytHc1VqQ3VoQTI2V0tsbVNZU3VC?=
 =?utf-8?B?MkQ0c2hhZkNydnFkNmJKQ3B0RVpSWnRRa21IYVd1MGJxVFJyYk96SHB0ZzJV?=
 =?utf-8?B?OG43MktBNzhxNzVyall1aHhrOTJ0SXpwOUVUcnF0OGs1aGg1MTRMaWlncUQ5?=
 =?utf-8?B?bDR0YmdwY1VxSkVTT1RjN2dncmRUU1RWSC81aXVHTnhpYjJQSDlFNHdOWmRl?=
 =?utf-8?B?V3FKT1M3TTRQaStGTFZsbTNHTW9SWnZWOGZ4UXVFZ1U0RWVoNVA2Tlp0cklC?=
 =?utf-8?B?ZnJsdDFvVktyaGE3TGU4My93ekp4a0ppamFCS3NLc3VJbmI0QWp0aFpwTTli?=
 =?utf-8?B?OE1CUWxuRzhzRllTWnBEazlRTWtwQ0QwTWhFRjBxSnYrU1pRaDJYNHlPQW01?=
 =?utf-8?B?QjliQTdXNVRYei9HOEFDeUM1clFIQVZMdEdoUU1QTnZNTFVoY1pTL3RYTUds?=
 =?utf-8?B?WGNFNEtPTnlqRFJYUmVDV1B5MlI2WGZSazNLbzZESGIzbytLeG8zNThDd1Fk?=
 =?utf-8?B?cnJJbXBhQStKUkVyU04xTm01c3JLb3NjcGoyMU16T0I5VnBma0dSdmNPNHJk?=
 =?utf-8?B?WW9tZ1gwVTc3RUJCUlJiQjFOLzR6YURDZUFrSWR1ajhHQXNoRnJ6T3hmb2Nw?=
 =?utf-8?B?QVRBYUR3NmxvclNjL2MxK1RtUG56VXloT20vdVgvNFVRKzlqR3JKT3gvaTNa?=
 =?utf-8?B?R2thZEhXZUtvTVZIUTFjLzRRN1ZHT2VTc29idFMyTzErS3V3anFRa1lyVVR4?=
 =?utf-8?B?b0FLSEx1K3JyckFDTHR2NXBMdlVrbElVQmg0TDBXdDZkY0dsbGpFcXNmMzZN?=
 =?utf-8?B?dm81cVlRQ2pNdnBRT2k5NEhRbjQ1VjhBaUo4VWhHOCtqUXVrU0xqVmVSalhy?=
 =?utf-8?B?MUdWdDZobVRiL01rcXd1R2JOR29YRHgwMlZKUE9TWngwMU9QOXVVUnJIdTJn?=
 =?utf-8?B?RzN4S21Ud3NDRDA1djUxS3FybnFtMDNCZTc1MWdvSWcwWGd2NE9ES3JTQUFH?=
 =?utf-8?B?d3VDTXRHWmRqdXhUVFRRSkZOMTBuL2lTK1ZDOFIrWVRvSnpqcEFoNklnMThh?=
 =?utf-8?B?dDF2NWxjYkVEZ0kwRkpYWHZRd3M0anRQTGlFb3JUMERlZXBYSEZqeEI1dUE5?=
 =?utf-8?B?MWhobExuRHhkc3g1MXkzN1VjZTJKdE9LUjV1YnNoblFDMmVONjgzQyt5empY?=
 =?utf-8?B?SDZmQnZ0b2FNNHZoQ3N0YWFGcHZIc3ZaQWZiWWtBaVNhdVFkL2RoRzB5bjgz?=
 =?utf-8?Q?GdRTP8a1ptGZlxLo4xpMSgE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7c4527f8-0c5b-46cf-8c53-08db2fef49a9
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB2987.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Mar 2023 00:48:18.4380
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Z9TE3p8CwFrcY/QIpcDd+qGVyiUofJwCYNW+QzeCrAPXFUdIcDK2PCin9C1a+PCHSEI3NLIu0RmwpkXDJTKJGkuHNwerkZ3RWHaKQXL2IGk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR11MB4709
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-0.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_NONE
        autolearn=unavailable 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

On Tue, Mar 28, 2023 at 02:08:47PM +0100, Tvrtko Ursulin wrote:
>
>On 28/03/2023 10:36, Min Li wrote:
>>Userspace can guess the id value and try to race oa_config object creation
>>with config remove, resulting in a use-after-free if we dereference the
>>object after unlocking the metrics_lock.  For that reason, unlocking the
>>metrics_lock must be done after we are done dereferencing the object.
>>
>>Signed-off-by: Min Li <lm0963hack@gmail.com>
>
>Fixes: f89823c21224 ("drm/i915/perf: Implement I915_PERF_ADD/REMOVE_CONFIG interface")
>Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>Cc: <stable@vger.kernel.org> # v4.14+
>
>>---
>>  drivers/gpu/drm/i915/i915_perf.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>>diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>>index 824a34ec0b83..93748ca2c5da 100644
>>--- a/drivers/gpu/drm/i915/i915_perf.c
>>+++ b/drivers/gpu/drm/i915/i915_perf.c
>>@@ -4634,13 +4634,13 @@ int i915_perf_add_config_ioctl(struct drm_device *dev, void *data,
>>  		err = oa_config->id;
>>  		goto sysfs_err;
>>  	}
>>-
>>-	mutex_unlock(&perf->metrics_lock);
>>+	id = oa_config->id;
>>  	drm_dbg(&perf->i915->drm,
>>  		"Added config %s id=%i\n", oa_config->uuid, oa_config->id);
>>+	mutex_unlock(&perf->metrics_lock);
>>-	return oa_config->id;
>>+	return id;
>>  sysfs_err:
>>  	mutex_unlock(&perf->metrics_lock);
>
>LGTM.
>
>Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
>Umesh or Lionel could you please double check? I can merge if confirmed okay.

LGTM,

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Thanks,
Umesh

>
>Regards,
>
>Tvrtko