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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9A880C2BB3F for ; Mon, 20 Nov 2023 19:49:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231859AbjKTTtb (ORCPT ); Mon, 20 Nov 2023 14:49:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34898 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230059AbjKTTta (ORCPT ); Mon, 20 Nov 2023 14:49:30 -0500 Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.88]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F0F9FDC for ; Mon, 20 Nov 2023 11:49:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1700509766; x=1732045766; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=3CfVBHprN3F2efur8G3TZVr5oodSTmFDVzg3hC2rSbY=; b=AYA168H9tPlZqGbVRsXshEkHLETkZFD+twIbpzre/GmGp1Mld6bSQy7H r+k+ARCw/wbYZCF5OJgVQR2N86eMzN7CBBLr6ncdbTtyotYUo5XeRb3mh CwZF/ZFv0T3o10ZHxstvwZd72sgttJdlfCdYd9sECKqmagb+nXVM/oQgX DMGiIkdS6Vs/RRWSi/e/7PUx8x/be7at62ndyPxE+ybR1QHSDrX0CBrMA yTjjkPPcmftLSkLKYkfDsBFCCOt9BRQFGfN6GRuwIstF1Nvjwm6+9YpqW 2U8lUEnv6S8RktKo/9ViByOcaenH+buVFIsO2kglLEqWBmld+Akn39r3e g==; X-IronPort-AV: E=McAfee;i="6600,9927,10900"; a="422795680" X-IronPort-AV: E=Sophos;i="6.04,214,1695711600"; d="scan'208";a="422795680" Received: from orviesa001.jf.intel.com ([10.64.159.141]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Nov 2023 11:49:26 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.04,214,1695711600"; d="scan'208";a="14265648" Received: from aantonov-mobl1.ger.corp.intel.com (HELO [10.252.56.254]) ([10.252.56.254]) by smtpauth.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Nov 2023 11:49:25 -0800 Message-ID: <50ce6fce-c2fc-4392-b405-5c9a7a93f061@linux.intel.com> Date: Mon, 20 Nov 2023 20:49:05 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] perf/x86/intel/uncore: Fix NULL pointer dereference issue in upi_fill_topology() To: "Liang, Kan" , peterz@infradead.org, linux-kernel@vger.kernel.org Cc: kyle.meyer@hpe.com, alexey.v.bayduraev@linux.intel.com References: <20231115151327.1874060-1-alexander.antonov@linux.intel.com> Content-Language: en-US From: Alexander Antonov In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/15/2023 8:00 PM, Liang, Kan wrote: > > On 2023-11-15 10:13 a.m., alexander.antonov@linux.intel.com wrote: >> From: Alexander Antonov >> >> The NULL dereference happens inside upi_fill_topology() procedure in >> case of disabling one of the sockets on the system. >> >> For example, if you disable the 2nd socket on a 4-socket system then >> uncore_max_dies() returns 3 and inside pmu_alloc_topology() memory will >> be allocated only for 3 sockets and stored in type->topology. >> In discover_upi_topology() memory is accessed by socket id from CPUNODEID >> registers which contain physical ids (from 0 to 3) and on the line: >> >>     upi = &type->topology[nid][idx]; >> >> out-of-bound access will happen and the 'upi' pointer will be passed to >> upi_fill_topology() where it will be dereferenced. >> >> To avoid this issue update the code to convert physical socket id to >> logical socket id in discover_upi_topology() before accessing memory. >> >> Fixes: f680b6e6062e ("perf/x86/intel/uncore: Enable UPI topology discovery for Icelake Server") >> Reported-by: Kyle Meyer >> Tested-by: Kyle Meyer >> Signed-off-by: Alexander Antonov >> --- >> arch/x86/events/intel/uncore_snbep.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c >> index 8250f0f59c2b..49bc27ab26ad 100644 >> --- a/arch/x86/events/intel/uncore_snbep.c >> +++ b/arch/x86/events/intel/uncore_snbep.c >> @@ -5596,7 +5596,7 @@ static int discover_upi_topology(struct intel_uncore_type *type, int ubox_did, i >> struct pci_dev *ubox = NULL; >> struct pci_dev *dev = NULL; >> u32 nid, gid; >> - int i, idx, ret = -EPERM; >> + int i, idx, lgc_pkg, ret = -EPERM; >> struct intel_uncore_topology *upi; >> unsigned int devfn; >> >> @@ -5614,8 +5614,13 @@ static int discover_upi_topology(struct intel_uncore_type *type, int ubox_did, i >> for (i = 0; i < 8; i++) { >> if (nid != GIDNIDMAP(gid, i)) >> continue; >> + lgc_pkg = topology_phys_to_logical_pkg(i); >> + if (lgc_pkg < 0) { >> + ret = -EPERM; >> + goto err; >> + } > In the snbep_pci2phy_map_init(), there are similar codes to find the > logical die id. Can we factor a common function for both of them? > > Thanks, > Kan Hi Kan, Thank you for your comment. Yes, I think we can factor out the common loop where GIDNIDMAP is being checked. But inside snbep_pci2phy_map_init() we have a bit different procedure which also does the following: if (topology_max_die_per_package() > 1)     die_id = i; I think that having this code, at least, in our case could bring us to the same issue which we are trying to fix. But of course we could parametrize this checking. What do you think? Thanks, Alexander > >> for (idx = 0; idx < type->num_boxes; idx++) { >> - upi = &type->topology[nid][idx]; >> + upi = &type->topology[lgc_pkg][idx]; >> devfn = PCI_DEVFN(dev_link0 + idx, ICX_UPI_REGS_ADDR_FUNCTION); >> dev = pci_get_domain_bus_and_slot(pci_domain_nr(ubox->bus), >> ubox->bus->number, >> @@ -5626,6 +5631,7 @@ static int discover_upi_topology(struct intel_uncore_type *type, int ubox_did, i >> goto err; >> } >> } >> + break; >> } >> } >> err: >> >> base-commit: 9bacdd8996c77c42ca004440be610692275ff9d0