Re: [RFC PATCH] arm64/efi: isolate EFI stub from the kernel proper

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

Hi Ard,

On Thu, Sep 17, 2015 at 11:51:07AM +0100, Ard Biesheuvel wrote:
> On 15 September 2015 at 17:24, Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On 15 September 2015 at 16:46, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
> >> On Tue, Sep 15, 2015 at 11:11:43AM +0100, Ard Biesheuvel wrote:
> >>> Since arm64 does not use a builtin decompressor, the EFI stub is built
> >>> into the kernel proper. So far, this has been working fine, but actually,
> >>> since the stub is in fact a PE/COFF relocatable binary that is executed
> >>> at an unknown offset in the 1:1 mapping provided by the UEFI firmware, we
> >>> should not be seamlessly sharing code with the kernel proper, which is a
> >>> position dependent executable linked at a high virtual offset.
> >>>
> >>> So instead, separate the contents of libstub and its dependencies, by
> >>> putting them into their own namespace by prefixing all of its symbols
> >>> with __efistub. This way, we have tight control over what parts of the
> >>> kernel proper are referenced by the stub.
> >>
> >> Could we add an __efistub annotation to spit out warnings if the stub
> >> calls into unexpected kernel code, like we do for __init/__ref?
> >>
> >
> > Currently, it will break the build rather than warn if you use a
> > disallowed symbol, which I think is not unreasonable.
> >
> > But I suppose that the objcopy step in this patch could rename the
> > sections to .efistub.xxx sections, which would allow us to reuse some
> > of the section mismatch code.
> > However, this would involve marking things like the generic string and
> > memcpy routines __efistub as well, which I think may be too much.
> > Also, note that the logic is inverted here: with __init, we disallow
> > normal code to call __init functions, but with __efistub, it will be
> > the other way around, which may be more difficult to accomplish
> > (Rutland and I did discuss this option when we talked about this over
> > IRC)
> >
> 
> OK, I have given this a go, and as it turns out, it implies that we go
> and mark generic pieces of lib/ as __section(.text.efistubok) in order
> for modpost.c to accept it. Tweaking modpost.c itself seems quite
> doable, since the logic is fairly flexible and can easily be augmented
> to complain about unauthorized references from the stub to the kernel
> proper.
> 
> So what we could do is fold libfdt and lib/sort.c (which are the
> primary generic dependencies) into the stub, but we would still need
> to retain the symbol prefixing bit to prevent the stub's symbols from
> clashing with the ones from the kernel proper. And with the symbol
> prefixing in place, we have something that is even stronger than
> section mismatch, which is to error out on undefined references rather
> than warn about section mismatches.
> 
> I think the current approach is better, but only if we agree that we
> should do something in the first place. (Currently, there are no known
> issues, just the awareness that things are not quite as tidy as they
> should be)

I agree, but thanks for looking into it. The only downside I still see
with symbol namespacing is that the error produced won't be as obvious
as something akin to a section mismatch, but then again, it's mainly you
hacking on the stub so it's not really an issue :)

Will