Re: [PATCH 07/13] x86: Secure Launch kernel early boot stub

2020-10-21 Thread Daniel Kiper
On Mon, Oct 19, 2020 at 01:18:22PM -0400, Arvind Sankar wrote:
> On Mon, Oct 19, 2020 at 04:51:53PM +0200, Daniel Kiper wrote:
> > On Fri, Oct 16, 2020 at 04:51:51PM -0400, Arvind Sankar wrote:
> > > On Thu, Oct 15, 2020 at 08:26:54PM +0200, Daniel Kiper wrote:
> > > >
> > > > I am discussing with Ross the other option. We can create
> > > > .rodata.mle_header section and put it at fixed offset as
> > > > kernel_info is. So, we would have, e.g.:
> > > >
> > > > arch/x86/boot/compressed/vmlinux.lds.S:
> > > > .rodata.kernel_info KERNEL_INFO_OFFSET : {
> > > > *(.rodata.kernel_info)
> > > > }
> > > > ASSERT(ABSOLUTE(kernel_info) == KERNEL_INFO_OFFSET, 
> > > > "kernel_info at bad address!")
> > > >
> > > > .rodata.mle_header MLE_HEADER_OFFSET : {
> > > > *(.rodata.mle_header)
> > > > }
> > > > ASSERT(ABSOLUTE(mle_header) == MLE_HEADER_OFFSET, "mle_header 
> > > > at bad address!")
> > > >
> > > > arch/x86/boot/compressed/sl_stub.S:
> > > > #define mleh_rva(X) (((X) - mle_header) + MLE_HEADER_OFFSET)
> > > >
> > > > .section ".rodata.mle_header", "a"
> > > >
> > > > SYM_DATA_START(mle_header)
> > > > .long   0x9082ac5a/* UUID0 */
> > > > .long   0x74a7476f/* UUID1 */
> > > > .long   0xa2555c0f/* UUID2 */
> > > > .long   0x42b651cb/* UUID3 */
> > > > .long   0x0034/* MLE header size */
> > > > .long   0x00020002/* MLE version 2.2 */
> > > > .long   mleh_rva(sl_stub_entry)/* Linear entry point of MLE 
> > > > (virt. address) */
> > > > .long   0x/* First valid page of MLE */
> > > > .long   0x/* Offset within binary of first byte of 
> > > > MLE */
> > > > .long   0x/* Offset within binary of last byte + 1 
> > > > of MLE */
> > > > .long   0x0223/* Bit vector of MLE-supported 
> > > > capabilities */
> > > > .long   0x/* Starting linear address of command 
> > > > line (unused) */
> > > > .long   0x/* Ending linear address of command line 
> > > > (unused) */
> > > > SYM_DATA_END(mle_header)
> > > >
> > > > Of course MLE_HEADER_OFFSET has to be defined as a constant somewhere.
> > > > Anyway, is it acceptable?
> >
> > What do you think about my MLE_HEADER_OFFSET and related stuff proposal?
> >
>
> I'm wondering if it would be easier to just allow relocations in these
> special "header" sections. I need to check how easy/hard it is to do
> that without triggering linker warnings.

Ross and I still bouncing some ideas. We came to the conclusion that
putting mle_header into kernel .rodata.kernel_info section or even
arch/x86/boot/compressed/kernel_info.S file would be the easiest thing
to do at this point. Of course I would suggest some renaming too. E.g.
.rodata.kernel_info to .rodata.kernel_headers, etc. Does it make sense
for you?

Daniel
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH 07/13] x86: Secure Launch kernel early boot stub

2020-10-19 Thread Daniel Kiper
On Fri, Oct 16, 2020 at 04:51:51PM -0400, Arvind Sankar wrote:
> On Thu, Oct 15, 2020 at 08:26:54PM +0200, Daniel Kiper wrote:
> >
> > I am discussing with Ross the other option. We can create
> > .rodata.mle_header section and put it at fixed offset as
> > kernel_info is. So, we would have, e.g.:
> >
> > arch/x86/boot/compressed/vmlinux.lds.S:
> > .rodata.kernel_info KERNEL_INFO_OFFSET : {
> > *(.rodata.kernel_info)
> > }
> > ASSERT(ABSOLUTE(kernel_info) == KERNEL_INFO_OFFSET, "kernel_info at 
> > bad address!")
> >
> > .rodata.mle_header MLE_HEADER_OFFSET : {
> > *(.rodata.mle_header)
> > }
> > ASSERT(ABSOLUTE(mle_header) == MLE_HEADER_OFFSET, "mle_header at 
> > bad address!")
> >
> > arch/x86/boot/compressed/sl_stub.S:
> > #define mleh_rva(X) (((X) - mle_header) + MLE_HEADER_OFFSET)
> >
> > .section ".rodata.mle_header", "a"
> >
> > SYM_DATA_START(mle_header)
> > .long   0x9082ac5a/* UUID0 */
> > .long   0x74a7476f/* UUID1 */
> > .long   0xa2555c0f/* UUID2 */
> > .long   0x42b651cb/* UUID3 */
> > .long   0x0034/* MLE header size */
> > .long   0x00020002/* MLE version 2.2 */
> > .long   mleh_rva(sl_stub_entry)/* Linear entry point of MLE 
> > (virt. address) */
> > .long   0x/* First valid page of MLE */
> > .long   0x/* Offset within binary of first byte of MLE 
> > */
> > .long   0x/* Offset within binary of last byte + 1 of 
> > MLE */
> > .long   0x0223/* Bit vector of MLE-supported capabilities */
> > .long   0x/* Starting linear address of command line 
> > (unused) */
> > .long   0x/* Ending linear address of command line 
> > (unused) */
> > SYM_DATA_END(mle_header)
> >
> > Of course MLE_HEADER_OFFSET has to be defined as a constant somewhere.
> > Anyway, is it acceptable?

What do you think about my MLE_HEADER_OFFSET and related stuff proposal?

> > There is also another problem. We have to put into mle_header size of
> > the Linux kernel image. Currently it is done by the bootloader but
> > I think it is not a role of the bootloader. The kernel image should
> > provide all data describing its properties and do not rely on the
> > bootloader to do that. Ross and I investigated various options but we
> > did not find a good/simple way to do that. Could you suggest how we
> > should do that or at least where we should take a look to get some
> > ideas?
> >
> > Daniel
>
> What exactly is the size you need here? Is it just the size of the
> protected mode image, that's startup_32 to _edata. Or is it the size of
> the whole bzImage file, or something else? I guess the same question
> applies to "first valid page of MLE" and "first byte of MLE", and the
> linear entry point -- are those all relative to startup_32 or do they
> need to be relative to the start of the bzImage, i.e. you have to add
> the size of the real-mode boot stub?
>
> If you need to include the size of the bzImage file, that's not known
> when the files in boot/compressed are built. It's only known after the
> real-mode stub is linked. arch/x86/boot/tools/build.c fills in various
> details in the setup header and creates the bzImage file, but it does
> not currently modify anything in the protected-mode portion of the
> compressed kernel (i.e. arch/x86/boot/compressed/vmlinux, which then
> gets converted to binary format as arch/x86/boot/vmlinux.bin), so it
> would need to be extended if you need to modify the MLE header to
> include the bzImage size or anything depending on the size of the
> real-mode stub.

Ross clarified this. So, I not have to add much here.

Daniel
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [PATCH 07/13] x86: Secure Launch kernel early boot stub

2020-10-15 Thread Daniel Kiper
On Tue, Sep 29, 2020 at 10:03:47AM -0400, Ross Philipson wrote:
> On 9/25/20 3:18 PM, Arvind Sankar wrote:

[...]

> > You should see them if you do
> > readelf -r arch/x86/boot/compressed/vmlinux
> >
> > In terms of the code, things like:
> >
> > addl%ebx, (sl_gdt_desc + 2)(%ebx)
> >
> > will create a relocation, because the linker interprets this as wanting
> > the runtime address of sl_gdt_desc, rather than just the offset from
> > startup_32.
> >
> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/arch/x86/boot/compressed/head_64.S*n48__;Iw!!GqivPVa7Brio!JpZWv1cCPZdjD2jbCCGT7P9UIVl_lhX7YjckAnUcvi927jwZI7X3nX0MpIAZOyktJds$
> >
> > has a comment with some explanation and a macro that the 32-bit code in
> > startup_32 uses to avoid creating relocations.
> >
> > Since the SL code is in a different assembler file (and a different
> > section), you can't directly use the same macro. I would suggest getting
> > rid of sl_stub_entry and entering directly at sl_stub, and then the code
> > in sl_stub.S can use sl_stub for the base address, defining the rva()
> > macro there as
> >
> > #define rva(X) ((X) - sl_stub)
> >
> > You will also need to avoid initializing data with symbol addresses.
> >
> > .long mle_header
> > .long sl_stub_entry
> > .long sl_gdt
> >
> > will create relocations. The third one is easy, just replace it with
> > sl_gdt - sl_gdt_desc and initialize it at runtime with
> >
> > lealrva(sl_gdt_desc)(%ebx), %eax
> > addl%eax, 2(%eax)
> > lgdt(%eax)
> >
> > The other two are more messy, unfortunately there is no easy way to tell
> > the linker what we want here. The other entry point addresses (for the
> > EFI stub) are populated in a post-processing step after the compressed
> > kernel has been linked, we could teach it to also update kernel_info.
> >
> > Without that, for kernel_info, you could change it to store the offset
> > of the MLE header from kernel_info, instead of from the start of the
> > image.
> >
> > For the MLE header, it could be moved to .head.text in head_64.S, and
> > initialized with
> > .long rva(sl_stub)
> > This will also let it be placed at a fixed offset from startup_32, so
> > that kernel_info can just be populated with a constant.

I am discussing with Ross the other option. We can create
.rodata.mle_header section and put it at fixed offset as
kernel_info is. So, we would have, e.g.:

arch/x86/boot/compressed/vmlinux.lds.S:
.rodata.kernel_info KERNEL_INFO_OFFSET : {
*(.rodata.kernel_info)
}
ASSERT(ABSOLUTE(kernel_info) == KERNEL_INFO_OFFSET, "kernel_info at bad 
address!")

.rodata.mle_header MLE_HEADER_OFFSET : {
*(.rodata.mle_header)
}
ASSERT(ABSOLUTE(mle_header) == MLE_HEADER_OFFSET, "mle_header at bad 
address!")

arch/x86/boot/compressed/sl_stub.S:
#define mleh_rva(X) (((X) - mle_header) + MLE_HEADER_OFFSET)

.section ".rodata.mle_header", "a"

SYM_DATA_START(mle_header)
.long   0x9082ac5a/* UUID0 */
.long   0x74a7476f/* UUID1 */
.long   0xa2555c0f/* UUID2 */
.long   0x42b651cb/* UUID3 */
.long   0x0034/* MLE header size */
.long   0x00020002/* MLE version 2.2 */
.long   mleh_rva(sl_stub_entry)/* Linear entry point of MLE (virt. 
address) */
.long   0x/* First valid page of MLE */
.long   0x/* Offset within binary of first byte of MLE */
.long   0x/* Offset within binary of last byte + 1 of MLE */
.long   0x0223/* Bit vector of MLE-supported capabilities */
.long   0x/* Starting linear address of command line 
(unused) */
.long   0x/* Ending linear address of command line (unused) 
*/
SYM_DATA_END(mle_header)

Of course MLE_HEADER_OFFSET has to be defined as a constant somewhere.
Anyway, is it acceptable?

There is also another problem. We have to put into mle_header size of
the Linux kernel image. Currently it is done by the bootloader but
I think it is not a role of the bootloader. The kernel image should
provide all data describing its properties and do not rely on the
bootloader to do that. Ross and I investigated various options but we
did not find a good/simple way to do that. Could you suggest how we
should do that or at least where we should take a look to get some
ideas?

Daniel
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu


Re: [RFC PATCH v1 10/18] x86/efi: Access EFI related tables in the clear

2016-05-25 Thread Daniel Kiper
On Tue, May 24, 2016 at 09:54:31AM -0500, Tom Lendacky wrote:
> On 05/12/2016 01:20 PM, Tom Lendacky wrote:
> > On 05/10/2016 08:57 AM, Borislav Petkov wrote:
> >> On Tue, May 10, 2016 at 02:43:58PM +0100, Matt Fleming wrote:
> >>> Is it not possible to maintain some kind of kernel virtual address
> >>> mapping so memremap*() and friends can figure out when to twiddle the
> >>> mapping attributes and map with/without encryption?
> >>
> >> I guess we can move the sme_* specific stuff one indirection layer
> >> below, i.e., in the *memremap() routines so that callers don't have to
> >> care... That should keep the churn down...
> >>
> >
> > We could do that, but we'll have to generate that list of addresses so
> > that it can be checked against the range being mapped.  Since this is
> > part of early memmap support searching that list every time might not be
> > too bad. I'll have to look into that and see what that looks like.
>
> I looked into this and this would be a large change also to parse tables
> and build lists.  It occurred to me that this could all be taken care of
> if the early_memremap calls were changed to early_ioremap calls. Looking
> in the git log I see that they were originally early_ioremap calls but
> were changed to early_memremap calls with this commit:
>
> commit abc93f8eb6e4 ("efi: Use early_mem*() instead of early_io*()")
>
> Looking at the early_memremap code and the early_ioremap code they both
> call __early_ioremap so I don't see how this change makes any
> difference (especially since FIXMAP_PAGE_NORMAL and FIXMAP_PAGE_IO are
> identical in this case).
>
> Is it safe to change these back to early_ioremap calls (at least on
> x86)?

Commit f955371ca9d3986bca100666041fcfa9b6d21962 (x86: remove the Xen-specific
_PAGE_IOMAP PTE flag) made commit abc93f8eb6e4 unnecessary. Though, IMO, it
is still valid code cleanup. So, if it is not very strongly needed I would
not revert this change.

Daniel
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu