linux-kernel - Re: [PATCH] firmware: arm_sdei: pass sdei_api_event

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] [day] [month] [year] [list]

Message-ID: <07f80973-9310-60d4-9828-4d3ae2112ad3@linux.alibaba.com>
Date:   Mon, 11 Oct 2021 18:37:01 +0800
From:   乱石 <zhangliguang@...ux.alibaba.com>
To:     James Morse <james.morse@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] firmware: arm_sdei: pass sdei_api_event_register right
 parameters

Hi James,

在 2021/10/9 1:39, James Morse 写道:
> Hello!
>
> (sorry for the delayed response)
>
> On 10/09/2021 05:01, Liguang Zhang wrote:
>> Function _local_event_enable is used for private sdei event
>> registeration called by sdei_event_register. We should pass
> (registration)
>
>> sdei_api_event_register right flag and mpidr parameters, otherwise atf
>> may trigger assert errors.
>> diff --git a/drivers/firmware/arm_sdei.c b/drivers/firmware/arm_sdei.c
>> index a7e762c352f9..0736752dadde 100644
>> --- a/drivers/firmware/arm_sdei.c
>> +++ b/drivers/firmware/arm_sdei.c
>> @@ -558,14 +558,16 @@ static int sdei_api_event_register(u32 event_num, unsigned long entry_point,
>>   static void _local_event_register(void *data)
>>   {
>>   	int err;
>> +	u64 mpidr;
>>   	struct sdei_registered_event *reg;
>>   	struct sdei_crosscall_args *arg = data;
>>   
>>   	WARN_ON(preemptible());
>>   
>> +	mpidr = read_cpuid_mpidr();
>>   	reg = per_cpu_ptr(arg->event->private_registered, smp_processor_id());
>>   	err = sdei_api_event_register(arg->event->event_num, sdei_entry_point,
>> -				      reg, 0, 0);
>> +				      reg, SDEI_EVENT_REGISTER_RM_PE, mpidr);
> Hmmm, this looks like a bug in TFA.
>
> 5.1.2.2 "Parameters" of DEN 0054B has:
> | Routing mode is valid only for a shared event. For a private event, the routing mode is
> | ignored.
>
> Worse, the mpidr field has:
> | Currently the format is defined only when the selected routing mode is RM_PE.

or a private event, we route SDEI_EVENT_REGISTER_RM_PE and mpidr 
parameters may be more rationable.


>
>
> Over in trusted firmware land:
>
> https://git.trustedfirmware.org/TF-A/trusted-firmware-a.git/tree/services/std_svc/sdei/sdei_main.c?h=v2.5#n361
>
> | static int64_t sdei_event_register(int ev_num,
> | 				uint64_t ep,
> | 				uint64_t arg,
> | 				uint64_t flags,
> |				uint64_t mpidr)
> | {
>
> |	/* Private events always target the PE */
> |	if (is_event_private(map))
> |		flags = SDEI_REGF_RM_PE;
>
> It looks like this re-uses the 'caller specified the routing' code, but didn't update the
> mpidr.
>
>
> You mention TFA takes an assert failure, I assume that brings the machine down.
> (Presumably you don't have a CPU with an affinity of zero.)

Yes, that brings the machine down. In opensource ATF, CPU with an 
affinity of zero.

The problem backaround:

we use local secure arch timer as sdei watchdog timer for hardlockup 
detection, in  os panic flow, we mask sdei event, then trigger the assert


if (se->reg_flags == SDEI_REGF_RM_PE)

     assert(se->affinity == my_mpidr);


Thanks,

Liguang

>
> Does this mean no-one relies on this, and we can fix the firmware?
> (I'll go report this to the relevant folk)
>
>
> Thanks!
>
> James
>
>
>>   
>>   	sdei_cross_call_return(arg, err);
>>   }
>>