linux-kernel - Re: [PATCH V4 03/11] coresight: tmc: Extract device properties from AMBA pid based table lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <44597c9a-79bd-40f8-87a7-b53582132583@arm.com>
Date: Mon, 12 Feb 2024 12:13:00 +0000
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Anshuman Khandual <anshuman.khandual@....com>,
 linux-arm-kernel@...ts.infradead.org
Cc: Lorenzo Pieralisi <lpieralisi@...nel.org>,
 Sudeep Holla <sudeep.holla@....com>, Mike Leach <mike.leach@...aro.org>,
 James Clark <james.clark@....com>,
 Maxime Coquelin <mcoquelin.stm32@...il.com>,
 Alexandre Torgue <alexandre.torgue@...s.st.com>, linux-acpi@...r.kernel.org,
 linux-kernel@...r.kernel.org, coresight@...ts.linaro.org,
 linux-stm32@...md-mailman.stormreply.com
Subject: Re: [PATCH V4 03/11] coresight: tmc: Extract device properties from
 AMBA pid based table lookup

On 23/01/2024 05:46, Anshuman Khandual wrote:
> This extracts device properties from AMBA pid based table lookup. This also
> defers tmc_etr_setup_caps() after the coresight device has been initialized
> so that PID value can be read.
> 
> Cc: Suzuki K Poulose <suzuki.poulose@....com>
> Cc: Mike Leach <mike.leach@...aro.org>
> Cc: James Clark <james.clark@....com>
> Cc: coresight@...ts.linaro.org
> Cc: linux-arm-kernel@...ts.infradead.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
> ---
>   .../hwtracing/coresight/coresight-tmc-core.c  | 19 +++++++++++++------
>   1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-core.c b/drivers/hwtracing/coresight/coresight-tmc-core.c
> index 7ec5365e2b64..e71db3099a29 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-core.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-core.c
> @@ -370,16 +370,24 @@ static inline bool tmc_etr_has_non_secure_access(struct tmc_drvdata *drvdata)
>   	return (auth & TMC_AUTH_NSID_MASK) == 0x3;
>   }
>   
> +#define TMC_AMBA_MASK 0xfffff
> +
> +static const struct amba_id tmc_ids[];
> +
>   /* Detect and initialise the capabilities of a TMC ETR */
> -static int tmc_etr_setup_caps(struct device *parent, u32 devid, void *dev_caps)
> +static int tmc_etr_setup_caps(struct device *parent, u32 devid)
>   {
>   	int rc;
> -	u32 dma_mask = 0;
> +	u32 tmc_pid, dma_mask = 0;
>   	struct tmc_drvdata *drvdata = dev_get_drvdata(parent);
> +	void *dev_caps;
>   
>   	if (!tmc_etr_has_non_secure_access(drvdata))
>   		return -EACCES;
>   
> +	tmc_pid = coresight_get_pid(&drvdata->csdev->access) & TMC_AMBA_MASK;
> +	dev_caps = coresight_get_uci_data_from_amba(tmc_ids, tmc_pid);
> +
>   	/* Set the unadvertised capabilities */
>   	tmc_etr_init_caps(drvdata, (u32)(unsigned long)dev_caps);
>   
> @@ -497,10 +505,6 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>   		desc.type = CORESIGHT_DEV_TYPE_SINK;
>   		desc.subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_SYSMEM;
>   		desc.ops = &tmc_etr_cs_ops;
> -		ret = tmc_etr_setup_caps(dev, devid,
> -					 coresight_get_uci_data(id));
> -		if (ret)
> -			goto out;
>   		idr_init(&drvdata->idr);
>   		mutex_init(&drvdata->idr_mutex);
>   		dev_list = &etr_devs;
> @@ -539,6 +543,9 @@ static int tmc_probe(struct amba_device *adev, const struct amba_id *id)
>   		goto out;
>   	}
>   
> +	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR)
> +		ret = tmc_etr_setup_caps(dev, devid);
> +

With this change, we silently accept an ETR that may only have "SECURE" 
access only and crash later while we try to enable tracing. You could
pass in the "access" (which is already in 'desc.access' in the original
call site and deal with it ?

Suzuki



>   	drvdata->miscdev.name = desc.name;
>   	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
>   	drvdata->miscdev.fops = &tmc_fops;