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@xxxxxxxxxx> wrote: > > On 15 September 2015 at 16:46, Will Deacon <will.deacon@xxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
