Re: [PATCH 17/17] efi/libstub/arm64: handle randomized TEXT_OFFSET

[Ard Biesheuvel]

On 14 May 2018 at 09:00, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> * Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>
>> On 14 May 2018 at 08:47, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>> >
>> > * Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>> >
>> >> From: Mark Rutland <mark.rutland@xxxxxxx>
>> >>
>> >> When CONFIG_RANDOMIZE_TEXT_OFFSET is selected, TEXT_OFFSET is an
>> >> arbitrary multiple of PAGE_SIZE in the interval [0, 2MB).
>> >>
>> >> The EFI stub does not account for the potential misalignment of
>> >> TEXT_OFFSET relative to EFI_KIMG_ALIGN, and produces a randomized
>> >> physical offset which is always a round multiple of EFI_KIMG_ALIGN.
>> >> This may result in statically allocated objects whose alignment exceeds
>> >> PAGE_SIZE to appear misaligned in memory. This has been observed to
>> >> result in spurious stack overflow reports and failure to make use of
>> >> the IRQ stacks, and theoretically could result in a number of other
>> >> issues.
>> >>
>> >> We can OR in the low bits of TEXT_OFFSET to ensure that we have the
>> >> necessary offset (and hence preserve the misalignment of TEXT_OFFSET
>> >> relative to EFI_KIMG_ALIGN), so let's do that.
>> >>
>> >> Fixes: 6f26b3671184c36d ("arm64: kaslr: increase randomization granularity")
>> >> Cc: <stable@xxxxxxxxxxxxxxx> # v4.7+
>> >> Reported-by: Kim Phillips <kim.phillips@xxxxxxx>
>> >> Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
>> >> Tested-by: Kim Phillips <kim.phillips@xxxxxxx>
>> >> [ardb: clarify commit log]
>> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
>> >> ---
>> >>  drivers/firmware/efi/libstub/arm64-stub.c | 7 +++++++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
>> >> index b9bd827caa22..541b82fdc8a2 100644
>> >> --- a/drivers/firmware/efi/libstub/arm64-stub.c
>> >> +++ b/drivers/firmware/efi/libstub/arm64-stub.c
>> >> @@ -97,6 +97,13 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
>> >>               u32 offset = !IS_ENABLED(CONFIG_DEBUG_ALIGN_RODATA) ?
>> >>                            (phys_seed >> 32) & mask : TEXT_OFFSET;
>> >>
>> >> +             /*
>> >> +              * With CONFIG_RANDOMIZE_TEXT_OFFSET, TEXT_OFFSET may not be a
>> >> +              * multiple of EFI_KIMG_ALIGN, and we must ensure that we apply
>> >> +              * the offset below EFI_KIMG_ALIGN.
>> >> +              */
>> >
>> > When referring to config variables in comments and changelogs I'd suggest a bit
>> > more verbosity:
>> >
>> >   s/CONFIG_RANDOMIZE_TEXT_OFFSET
>> >    /CONFIG_RANDOMIZE_TEXT_OFFSET=y
>> >
>> > ... because at first I thought (based on the name) that
>> > CONFIG_RANDOMIZE_TEXT_OFFSET is an actual integer offset value - while it's a
>> > bool. The =y makes the bool nature obvious.
>> >
>> > ( Similarly, when negated the canonical way to refer to it is
>> >   !CONFIG_RANDOMIZE_TEXT_OFFSET. )
>> >
>>
>> Fair enough.
>>
>> >> +             offset |= (TEXT_OFFSET % EFI_KIMG_ALIGN);
>> >
>> > The parentheses are not needed here I think.
>> >
>>
>> Nope.
>>
>> Will you fix this up when applying? Or should I resend?
>
> Since this was at the tail with no dependencies I'll skip this for now I think -
> mind sending the refreshed version in the next batch?
>

Actually, by the reasoning in your other reply, I'll fix this up and
resend it for -urgent instead.

Thanks,
Ard.