Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <ceb47045-3188-49ff-85b2-b37c9d0721e1@linux.intel.com>
Date:   Wed, 15 Nov 2023 14:00:16 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] perf/x86/intel/uncore: Fix NULL pointer dereference issue
 in upi_fill_topology()
Content-Language: en-US
To:     alexander.antonov@linux.intel.com, 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>
From:   "Liang, Kan" <kan.liang@linux.intel.com>
In-Reply-To: <20231115151327.1874060-1-alexander.antonov@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 2023-11-15 10:13 a.m., alexander.antonov@linux.intel.com wrote:
> From: Alexander Antonov <alexander.antonov@linux.intel.com>
> 
> 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 <kyle.meyer@hpe.com>
> Tested-by: Kyle Meyer <kyle.meyer@hpe.com>
> Signed-off-by: Alexander Antonov <alexander.antonov@linux.intel.com>
> ---
>  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

>  			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