| 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
| ||
|
Message-ID: <4002f56a-9541-cdf9-2663-fe27482e78d2@arm.com>
Date: Fri, 30 Jul 2021 12:29:51 +0100
From: Suzuki K Poulose <suzuki.poulose@....com>
To: Anshuman Khandual <anshuman.khandual@....com>,
coresight@...ts.linaro.org
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
tamas.zsoldos@....com, al.grant@....com, leo.yan@...aro.org,
mike.leach@...aro.org, mathieu.poirier@...aro.org,
jinlmao@....qualcomm.com
Subject: Re: [PATCH v2 02/10] coresight: etm4x: Use Trace Filtering controls
dynamically
On 30/07/2021 04:48, Anshuman Khandual wrote:
>
> On 7/23/21 6:16 PM, Suzuki K Poulose wrote:
>> The Trace Filtering support (FEAT_TRF) ensures that the ETM
>> can be prohibited from generating any trace for a given EL.
>> This is much stricter knob, than the TRCVICTLR exception level
>
> Could you please explain 'stricter' ? Are you suggesting that TRCVICTLR
> based exception filtering some times might not implement the filtering
> even if configured ?
>
Sure, the TRVICTLR only ensures that the ETM doesn't generate
any "branch" trace packets. But that doesn't prevent it from
generating the "Context" packets which may contain the kernel
addresses, if they are generated while in Kernel.
But, the FEAT_TRF strictly prevents the trace unit from generating
any packets while it is "prohibited". Thus it is a much better
control to prevent kernel address leaks via the trace.
>> masks. At the moment, we do a onetime enable trace at user and
>> kernel and leave it untouched for the kernel life time.
>>
>> This patch makes the switch dynamic, by honoring the filters
>> set by the user and enforcing them in the TRFCR controls.
>
> TRFCR actually helps in making the exception level filtering dynamic
> which was not possible earlier with TRCVICTLR.
>
>> We also rename the cpu_enable_tracing() appropriately to
>> cpu_detect_trace_filtering() and the drvdata member
>> trfc => trfcr to indicate the "value" of the TRFCR_EL1.
>
> Makes sense.
>
>>
>> Cc: Mathieu Poirier <mathieu.poirier@...aro.org>
>> Cc: Al Grant <al.grant@....com>
>> Cc: Mike Leach <mike.leach@...aro.org>
>> Cc: Leo Yan <leo.yan@...aro.org>
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>> ---
>> .../coresight/coresight-etm4x-core.c | 61 ++++++++++++++-----
>> drivers/hwtracing/coresight/coresight-etm4x.h | 5 +-
>> .../coresight/coresight-self-hosted-trace.h | 7 +++
>> 3 files changed, 55 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index 3e548dac9b05..adba84b29455 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -237,6 +237,43 @@ struct etm4_enable_arg {
>> int rc;
>> };
>>
>> +/*
>> + * etm4x_prohibit_trace - Prohibit the CPU from tracing at all ELs.
>> + * When the CPU supports FEAT_TRF, we could move the ETM to a trace
>> + * prohibited state by filtering the Exception levels via TRFCR_EL1.
>> + */
>> +static void etm4x_prohibit_trace(struct etmv4_drvdata *drvdata)
>> +{
>> + if (drvdata->trfcr)
>> + cpu_prohibit_trace();
>
> Should it be as etm4x_allow_trace() instead, where drvdata->trfcr
> indicates the presence of FEAT_TRF - just to be clear ?
>
> /* If the CPU doesn't support FEAT_TRF, nothing to do */
> if (!drvdata->trfcr)
> return;
>
> cpu_prohibit_trace();
>
OK
>> +}
>> +
>> +/*
>> + * etm4x_allow_trace - Allow CPU tracing in the respective ELs,
>> + * as configured by the drvdata->config.mode for the current
>> + * session. Even though we have TRCVICTLR bits to filter the
>> + * trace in the ELs, it doesn't prevent the ETM from generating
>> + * a packet (e.g, TraceInfo) that might contain the addresses from
>> + * the excluded levels. Thus we use the additional controls provided
>> + * via the Trace Filtering controls (FEAT_TRF) to make sure no trace
>> + * is generated for the excluded ELs.
>> + */
>> +static void etm4x_allow_trace(struct etmv4_drvdata *drvdata)
>> +{
>> + u64 trfcr = drvdata->trfcr;
>> +
>> + /* If the CPU doesn't support FEAT_TRF, nothing to do */
>> + if (!trfcr)
>> + return;
>> +
>> + if (drvdata->config.mode & ETM_MODE_EXCL_KERN)
>> + trfcr &= ~TRFCR_ELx_ExTRE;
>> + if (drvdata->config.mode & ETM_MODE_EXCL_USER)
>> + trfcr &= ~TRFCR_ELx_E0TRE;
>> +
>> + write_trfcr(trfcr);
>> +}
>> +
>> #ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>
>> #define HISI_HIP08_AMBA_ID 0x000b6d01
>> @@ -441,6 +478,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>> if (etm4x_is_ete(drvdata))
>> etm4x_relaxed_write32(csa, TRCRSR_TA, TRCRSR);
>>
>> + etm4x_allow_trace(drvdata);
>> /* Enable the trace unit */
>> etm4x_relaxed_write32(csa, 1, TRCPRGCTLR);
>>
>> @@ -719,7 +757,6 @@ static int etm4_enable(struct coresight_device *csdev,
>> static void etm4_disable_hw(void *info)
>> {
>> u32 control;
>> - u64 trfcr;
>> struct etmv4_drvdata *drvdata = info;
>> struct etmv4_config *config = &drvdata->config;
>> struct coresight_device *csdev = drvdata->csdev;
>> @@ -746,12 +783,7 @@ static void etm4_disable_hw(void *info)
>> * If the CPU supports v8.4 Trace filter Control,
>> * set the ETM to trace prohibited region.
>> */
>> - if (drvdata->trfc) {
>> - trfcr = read_sysreg_s(SYS_TRFCR_EL1);
>> - write_sysreg_s(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE),
>> - SYS_TRFCR_EL1);
>> - isb();
>> - }
>> + etm4x_prohibit_trace(drvdata);
>> /*
>> * Make sure everything completes before disabling, as recommended
>> * by section 7.3.77 ("TRCVICTLR, ViewInst Main Control Register,
>> @@ -767,9 +799,6 @@ static void etm4_disable_hw(void *info)
>> if (coresight_timeout(csa, TRCSTATR, TRCSTATR_PMSTABLE_BIT, 1))
>> dev_err(etm_dev,
>> "timeout while waiting for PM stable Trace Status\n");
>> - if (drvdata->trfc)
>> - write_sysreg_s(trfcr, SYS_TRFCR_EL1);
>> -
>> /* read the status of the single shot comparators */
>> for (i = 0; i < drvdata->nr_ss_cmp; i++) {
>> config->ss_status[i] =
>> @@ -964,15 +993,15 @@ static bool etm4_init_csdev_access(struct etmv4_drvdata *drvdata,
>> return false;
>> }
>>
>> -static void cpu_enable_tracing(struct etmv4_drvdata *drvdata)
>> +static void cpu_detect_trace_filtering(struct etmv4_drvdata *drvdata)
>> {
>> u64 dfr0 = read_sysreg(id_aa64dfr0_el1);
>> u64 trfcr;
>>
>> + drvdata->trfcr = 0;
>> if (!cpuid_feature_extract_unsigned_field(dfr0, ID_AA64DFR0_TRACE_FILT_SHIFT))
>> return;
>>
>> - drvdata->trfc = true;
>> /*
>> * If the CPU supports v8.4 SelfHosted Tracing, enable
>> * tracing at the kernel EL and EL0, forcing to use the
>> @@ -986,7 +1015,7 @@ static void cpu_enable_tracing(struct etmv4_drvdata *drvdata)
>> if (is_kernel_in_hyp_mode())
>> trfcr |= TRFCR_EL2_CX;
>>
>> - write_trfcr(trfcr);
>> + drvdata->trfcr = trfcr;
>> }
>>
>> static void etm4_init_arch_data(void *info)
>> @@ -1177,7 +1206,7 @@ static void etm4_init_arch_data(void *info)
>> /* NUMCNTR, bits[30:28] number of counters available for tracing */
>> drvdata->nr_cntr = BMVAL(etmidr5, 28, 30);
>> etm4_cs_lock(drvdata, csa);
>> - cpu_enable_tracing(drvdata);
>> + cpu_detect_trace_filtering(drvdata);
>> }
>>
>> static inline u32 etm4_get_victlr_access_type(struct etmv4_config *config)
>> @@ -1673,7 +1702,7 @@ static int etm4_cpu_save(struct etmv4_drvdata *drvdata)
>> int ret = 0;
>>
>> /* Save the TRFCR irrespective of whether the ETM is ON */
>> - if (drvdata->trfc)
>> + if (drvdata->trfcr)
>> drvdata->save_trfcr = read_trfcr();
>> /*
>> * Save and restore the ETM Trace registers only if
>> @@ -1782,7 +1811,7 @@ static void __etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>>
>> static void etm4_cpu_restore(struct etmv4_drvdata *drvdata)
>> {
>> - if (drvdata->trfc)
>> + if (drvdata->trfcr)
>> write_trfcr(drvdata->save_trfcr);
>> if (drvdata->state_needs_restore)
>> __etm4_cpu_restore(drvdata);
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index 82cba16b73a6..724819592c2e 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -919,7 +919,8 @@ struct etmv4_save_state {
>> * @nooverflow: Indicate if overflow prevention is supported.
>> * @atbtrig: If the implementation can support ATB triggers
>> * @lpoverride: If the implementation can support low-power state over.
>> - * @trfc: If the implementation supports Arm v8.4 trace filter controls.
>> + * @trfcr: If the CPU supportfs FEAT_TRF, value of the TRFCR_ELx with
>
> Typo here. ^^^^^^ s/supportfs/supports
>
>> + * trace allowed at user and kernel ELs. Otherwise, 0.
>
> The sentence here does not make sense. Is not the exception level ELx and EL0
> can be filtered out independently ? Should this be something like ...
The value holds a superset of the possible "allowed" configurations.
We do this to avoid setting the TRFCR_CX everytime depending on the
kernel EL (only possible from EL2). So we initialize the field
with value of TRCR_ELx with all the ELs enabled. This can be filtered
later by the driver accordingly. This will also serve as marker
to check the availability of the feature.
Thanks
Suzuki
>
> "If the CPU supports FEAT_TRF, value of the TRFCR_ELx - indicating whether
> trace is allowed at user [and/or] kernel ELs. Otherwise, 0."
>
>> * @config: structure holding configuration parameters.
>> * @save_trfcr: Saved TRFCR_EL1 register during a CPU PM event.
>> * @save_state: State to be preserved across power loss
>> @@ -972,7 +973,7 @@ struct etmv4_drvdata {
>> bool nooverflow;
>> bool atbtrig;
>> bool lpoverride;
>> - bool trfc;
>> + u64 trfcr;
>> struct etmv4_config config;
>> u64 save_trfcr;
>> struct etmv4_save_state *save_state;
>> diff --git a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
>> index 53b35a28075e..586d26e0cba3 100644
>> --- a/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
>> +++ b/drivers/hwtracing/coresight/coresight-self-hosted-trace.h
>> @@ -22,4 +22,11 @@ static inline void write_trfcr(u64 val)
>> isb();
>> }
>>
>> +static inline void cpu_prohibit_trace(void)
>> +{
>> + u64 trfcr = read_trfcr();
>> +
>> + /* Prohibit tracing at EL0 & the kernel EL */
>> + write_trfcr(trfcr & ~(TRFCR_ELx_ExTRE | TRFCR_ELx_E0TRE));
>> +}
>> #endif /* __CORESIGHT_SELF_HOSTED_TRACE_H */
>>
Powered by blists - more mailing lists