[dovetail][PATCH] arm64: dovetail: Notify the core about compat alignment fixups

[dovetail][PATCH] arm64: dovetail: Notify the core about compat alignment fixups

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

With CONFIG_COMPAT_ALIGNMENT_FIXUPS, misaligned multi-word loads and
stores of 32-bit tasks are emulated by do_compat_alignment_fixup(),
called from do_alignment_fault() via do_mem_abort(). The companion
core is only told about the trap when the fixup fails and
do_mem_abort() falls back to signal injection. If the fixup succeeds,
an oob thread runs the whole emulation from the oob stage: it fetches
the faulting instruction, emulates it by accessing user memory, which
may fault in pages, and accounts a perf event, none of which is
oob-safe. On arm, the equivalent path in do_alignment() is annotated.

Notify the core before attempting the fixup, reporting the trap as
ARM64_TRAP_ACCESS like do_bad_area() does for the same fault when the
fixups are disabled. If the fixup fails, the notification is unwound
before do_mem_abort() goes on, so its own annotation pairs as usual.

Assisted-by: opencode:Opus-5.5
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

This was a found by pure code analysis as most setups have
CONFIG_COMPAT_ALIGNMENT_FIXUPS off. It was not tested yet, working on a 
setup.

 arch/arm64/mm/fault.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 2726eb3fa2d46..c3532ff0440ad 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -867,9 +867,15 @@ static int __kprobes do_translation_fault(unsigned long far,
 static int do_alignment_fault(unsigned long far, unsigned long esr,
 			      struct pt_regs *regs)
 {
+	int ret;
+
 	if (IS_ENABLED(CONFIG_COMPAT_ALIGNMENT_FIXUPS) &&
-	    compat_user_mode(regs))
-		return do_compat_alignment_fixup(far, regs);
+	    compat_user_mode(regs)) {
+		mark_trap_entry(ARM64_TRAP_ACCESS, regs);
+		ret = do_compat_alignment_fixup(far, regs);
+		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
+		return ret;
+	}
 	do_bad_area(far, esr, regs);
 	return 0;
 }
-- 
2.47.3

Re: [dovetail][PATCH] arm64: dovetail: Notify the core about compat alignment fixups

[Philippe Gerum]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> With CONFIG_COMPAT_ALIGNMENT_FIXUPS, misaligned multi-word loads and
> stores of 32-bit tasks are emulated by do_compat_alignment_fixup(),
> called from do_alignment_fault() via do_mem_abort(). The companion
> core is only told about the trap when the fixup fails and
> do_mem_abort() falls back to signal injection. If the fixup succeeds,
> an oob thread runs the whole emulation from the oob stage: it fetches
> the faulting instruction, emulates it by accessing user memory, which
> may fault in pages, and accounts a perf event, none of which is
> oob-safe. On arm, the equivalent path in do_alignment() is annotated.
>
> Notify the core before attempting the fixup, reporting the trap as
> ARM64_TRAP_ACCESS like do_bad_area() does for the same fault when the
> fixups are disabled. If the fixup fails, the notification is unwound
> before do_mem_abort() goes on, so its own annotation pairs as usual.
>
> Assisted-by: opencode:Opus-5.5
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> This was a found by pure code analysis as most setups have
> CONFIG_COMPAT_ALIGNMENT_FIXUPS off. It was not tested yet, working on a 
> setup.
>
>  arch/arm64/mm/fault.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 2726eb3fa2d46..c3532ff0440ad 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -867,9 +867,15 @@ static int __kprobes do_translation_fault(unsigned long far,
>  static int do_alignment_fault(unsigned long far, unsigned long esr,
>  			      struct pt_regs *regs)
>  {
> +	int ret;
> +
>  	if (IS_ENABLED(CONFIG_COMPAT_ALIGNMENT_FIXUPS) &&
> -	    compat_user_mode(regs))
> -		return do_compat_alignment_fixup(far, regs);
> +	    compat_user_mode(regs)) {
> +		mark_trap_entry(ARM64_TRAP_ACCESS, regs);
> +		ret = do_compat_alignment_fixup(far, regs);
> +		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
> +		return ret;
> +	}
>  	do_bad_area(far, esr, regs);
>  	return 0;
>  }

If oob, we can expect MMF_DOVETAIL to be set for the current address
space, so .text pages of the caller should have been committed. In the
(unexpected) event some are not, get_user() and friends would still
recursively bump us to do_page_fault()/do_translation_fault() on a
subsequent synchronous exception path, which would mark the trap entry
eventually, causing the core to downgrade the task before damage is
done. This should work now that pipelined faults can pile up
safely. Anyway, I'll wait for some testing, things might be more
complicated.

-- 
Philippe.

Re: [dovetail][PATCH] arm64: dovetail: Notify the core about compat alignment fixups

[Jan Kiszka]

On 10.11.26 17:52, Philippe Gerum wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> With CONFIG_COMPAT_ALIGNMENT_FIXUPS, misaligned multi-word loads and
>> stores of 32-bit tasks are emulated by do_compat_alignment_fixup(),
>> called from do_alignment_fault() via do_mem_abort(). The companion
>> core is only told about the trap when the fixup fails and
>> do_mem_abort() falls back to signal injection. If the fixup succeeds,
>> an oob thread runs the whole emulation from the oob stage: it fetches
>> the faulting instruction, emulates it by accessing user memory, which
>> may fault in pages, and accounts a perf event, none of which is
>> oob-safe. On arm, the equivalent path in do_alignment() is annotated.
>>
>> Notify the core before attempting the fixup, reporting the trap as
>> ARM64_TRAP_ACCESS like do_bad_area() does for the same fault when the
>> fixups are disabled. If the fixup fails, the notification is unwound
>> before do_mem_abort() goes on, so its own annotation pairs as usual.
>>
>> Assisted-by: opencode:Opus-5.5
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>
>> This was a found by pure code analysis as most setups have
>> CONFIG_COMPAT_ALIGNMENT_FIXUPS off. It was not tested yet, working on a 
>> setup.
>>
>>  arch/arm64/mm/fault.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 2726eb3fa2d46..c3532ff0440ad 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -867,9 +867,15 @@ static int __kprobes do_translation_fault(unsigned long far,
>>  static int do_alignment_fault(unsigned long far, unsigned long esr,
>>  			      struct pt_regs *regs)
>>  {
>> +	int ret;
>> +
>>  	if (IS_ENABLED(CONFIG_COMPAT_ALIGNMENT_FIXUPS) &&
>> -	    compat_user_mode(regs))
>> -		return do_compat_alignment_fixup(far, regs);
>> +	    compat_user_mode(regs)) {
>> +		mark_trap_entry(ARM64_TRAP_ACCESS, regs);
>> +		ret = do_compat_alignment_fixup(far, regs);
>> +		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
>> +		return ret;
>> +	}
>>  	do_bad_area(far, esr, regs);
>>  	return 0;
>>  }
> 
> If oob, we can expect MMF_DOVETAIL to be set for the current address
> space, so .text pages of the caller should have been committed. In the
> (unexpected) event some are not, get_user() and friends would still
> recursively bump us to do_page_fault()/do_translation_fault() on a
> subsequent synchronous exception path, which would mark the trap entry
> eventually, causing the core to downgrade the task before damage is
> done. This should work now that pipelined faults can pile up
> safely. Anyway, I'll wait for some testing, things might be more
> complicated.
> 

Working on it. Key questions remain, though, why we have to (or not?) do
this differently on ARM and if things like the perf handling are
acttualy OOB-safe today. The LLM is rather brief here, I could also
challenge it while finishing the test case.

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center

Re: [dovetail][PATCH] arm64: dovetail: Notify the core about compat alignment fixups

[Jan Kiszka]

On 10.11.26 18:47, Jan Kiszka wrote:
> On 10.11.26 17:52, Philippe Gerum wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> With CONFIG_COMPAT_ALIGNMENT_FIXUPS, misaligned multi-word loads and
>>> stores of 32-bit tasks are emulated by do_compat_alignment_fixup(),
>>> called from do_alignment_fault() via do_mem_abort(). The companion
>>> core is only told about the trap when the fixup fails and
>>> do_mem_abort() falls back to signal injection. If the fixup succeeds,
>>> an oob thread runs the whole emulation from the oob stage: it fetches
>>> the faulting instruction, emulates it by accessing user memory, which
>>> may fault in pages, and accounts a perf event, none of which is
>>> oob-safe. On arm, the equivalent path in do_alignment() is annotated.
>>>
>>> Notify the core before attempting the fixup, reporting the trap as
>>> ARM64_TRAP_ACCESS like do_bad_area() does for the same fault when the
>>> fixups are disabled. If the fixup fails, the notification is unwound
>>> before do_mem_abort() goes on, so its own annotation pairs as usual.
>>>
>>> Assisted-by: opencode:Opus-5.5
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>> This was a found by pure code analysis as most setups have
>>> CONFIG_COMPAT_ALIGNMENT_FIXUPS off. It was not tested yet, working on a 
>>> setup.
>>>
>>>  arch/arm64/mm/fault.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>>> index 2726eb3fa2d46..c3532ff0440ad 100644
>>> --- a/arch/arm64/mm/fault.c
>>> +++ b/arch/arm64/mm/fault.c
>>> @@ -867,9 +867,15 @@ static int __kprobes do_translation_fault(unsigned long far,
>>>  static int do_alignment_fault(unsigned long far, unsigned long esr,
>>>  			      struct pt_regs *regs)
>>>  {
>>> +	int ret;
>>> +
>>>  	if (IS_ENABLED(CONFIG_COMPAT_ALIGNMENT_FIXUPS) &&
>>> -	    compat_user_mode(regs))
>>> -		return do_compat_alignment_fixup(far, regs);
>>> +	    compat_user_mode(regs)) {
>>> +		mark_trap_entry(ARM64_TRAP_ACCESS, regs);
>>> +		ret = do_compat_alignment_fixup(far, regs);
>>> +		mark_trap_exit(ARM64_TRAP_ACCESS, regs);
>>> +		return ret;
>>> +	}
>>>  	do_bad_area(far, esr, regs);
>>>  	return 0;
>>>  }
>>
>> If oob, we can expect MMF_DOVETAIL to be set for the current address
>> space, so .text pages of the caller should have been committed. In the
>> (unexpected) event some are not, get_user() and friends would still
>> recursively bump us to do_page_fault()/do_translation_fault() on a
>> subsequent synchronous exception path, which would mark the trap entry
>> eventually, causing the core to downgrade the task before damage is
>> done. This should work now that pipelined faults can pile up
>> safely. Anyway, I'll wait for some testing, things might be more
>> complicated.
>>
> 
> Working on it. Key questions remain, though, why we have to (or not?) do
> this differently on ARM and if things like the perf handling are
> acttualy OOB-safe today. The LLM is rather brief here, I could also
> challenge it while finishing the test case.
> 

And it's still an LLM: After poking a bit, it admits that there is no
major OOB problem, and the main reason for the patch is to align
reporting with what we do on 32-bit ARM. It still claims, though, that a
"sampling event (perf record -e alignment-faults, BPF program on it)" is
not OOB-safe:

"So the concrete failure needs a sampling alignment-faults event. With
CONFIG_DEBUG_IRQ_PIPELINE, check_inband_stage() then prints "IRQ
pipeline: some code running in oob context '…' called an in-band only
routine" with a stack dump. It also sets irq_pipeline_oopsing, which
turns off every later pipeline debug check. Without the debug option,
the irqsave guard's restore calls __inband_irq_enable() from oob, which
flips the stall bit and runs sync_current_irq_stage() on the oob stage.
That's an API violation, but I found no real damage beyond it. The BPF
case is worse: an oob thread can spin on a lock held by in-band code on
another CPU, with no bound on how long."

Jan

-- 
Siemens AG, Foundational Technologies
Linux Expert Center