| Message ID | 20180612052646.109214-4-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable basic sandbox support for EFI loader | expand |
On 12.06.18 07:26, Simon Glass wrote: > With sandbox the U-Boot code is not mapped into the sandbox memory range > so does not need to be excluded when allocating EFI memory. Update the EFI > memory init code to take account of that. > > Also use mapmem instead of a cast to convert a memory address to a > pointer. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: > - Update to use mapmem instead of a cast > > lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c > index ec66af98ea..210e49ee8b 100644 > --- a/lib/efi_loader/efi_memory.c > +++ b/lib/efi_loader/efi_memory.c > @@ -9,6 +9,7 @@ > #include <efi_loader.h> > #include <inttypes.h> > #include <malloc.h> > +#include <mapmem.h> > #include <watchdog.h> > #include <linux/list_sort.h> > > @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) > &t); > > if (r == EFI_SUCCESS) { > - struct efi_pool_allocation *alloc = (void *)(uintptr_t)t; > + struct efi_pool_allocation *alloc = map_sysmem(t, size); We want to eventually be able to run efi binaries inside sandbox, so we need to expose a 1:1 memory map inside the efi interfaces. That means the memory argument of efi_allocate_pages() already needs to be set to the virtual address in real VA space. The easiest way to get there is if you just put virtual addresses in the efi memory map. > alloc->num_pages = num_pages; > *buffer = alloc->data; > } > @@ -504,18 +505,22 @@ int efi_memory_init(void) > > efi_add_known_memory(); > > - /* Add U-Boot */ > - uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; > - uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; > - efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false); > - > - /* Add Runtime Services */ > - runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; > - runtime_end = (ulong)&__efi_runtime_stop; > - runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; > - runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; > - efi_add_memory_map(runtime_start, runtime_pages, > - EFI_RUNTIME_SERVICES_CODE, false); > + if (!IS_ENABLED(CONFIG_SANDBOX)) { > + /* Add U-Boot */ > + uboot_start = (gd->start_addr_sp - uboot_stack_size) & > + ~EFI_PAGE_MASK; > + uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; > + efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, > + false); I guess it's ok to hide U-Boot from the payload for sandbox, yes. Please extract the code into a function though, so that we don't get into too much indenting. > + > + /* Add Runtime Services */ > + runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; > + runtime_end = (ulong)&__efi_runtime_stop; > + runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; > + runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; > + efi_add_memory_map(runtime_start, runtime_pages, > + EFI_RUNTIME_SERVICES_CODE, false); > + } I guess we do want to indicate RTS, no? But like everything else we want to expose it with the real VA addresses. Alex > > #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER > /* Request a 32bit 64MB bounce buffer region */ >
Hi Alex, On 12 June 2018 at 02:05, Alexander Graf <agraf@suse.de> wrote: > > > On 12.06.18 07:26, Simon Glass wrote: >> With sandbox the U-Boot code is not mapped into the sandbox memory range >> so does not need to be excluded when allocating EFI memory. Update the EFI >> memory init code to take account of that. >> >> Also use mapmem instead of a cast to convert a memory address to a >> pointer. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> Changes in v5: None >> Changes in v4: None >> Changes in v3: None >> Changes in v2: >> - Update to use mapmem instead of a cast >> >> lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- >> 1 file changed, 18 insertions(+), 13 deletions(-) >> >> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >> index ec66af98ea..210e49ee8b 100644 >> --- a/lib/efi_loader/efi_memory.c >> +++ b/lib/efi_loader/efi_memory.c >> @@ -9,6 +9,7 @@ >> #include <efi_loader.h> >> #include <inttypes.h> >> #include <malloc.h> >> +#include <mapmem.h> >> #include <watchdog.h> >> #include <linux/list_sort.h> >> >> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) >> &t); >> >> if (r == EFI_SUCCESS) { >> - struct efi_pool_allocation *alloc = (void *)(uintptr_t)t; >> + struct efi_pool_allocation *alloc = map_sysmem(t, size); > > We want to eventually be able to run efi binaries inside sandbox, so we > need to expose a 1:1 memory map inside the efi interfaces. > > That means the memory argument of efi_allocate_pages() already needs to > be set to the virtual address in real VA space. The easiest way to get > there is if you just put virtual addresses in the efi memory map. Can you please explain that a bit more, or give a code example? I'm not quite sure what you mean. > >> alloc->num_pages = num_pages; >> *buffer = alloc->data; >> } >> @@ -504,18 +505,22 @@ int efi_memory_init(void) >> >> efi_add_known_memory(); >> >> - /* Add U-Boot */ >> - uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; >> - uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; >> - efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false); >> - >> - /* Add Runtime Services */ >> - runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; >> - runtime_end = (ulong)&__efi_runtime_stop; >> - runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; >> - runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; >> - efi_add_memory_map(runtime_start, runtime_pages, >> - EFI_RUNTIME_SERVICES_CODE, false); >> + if (!IS_ENABLED(CONFIG_SANDBOX)) { >> + /* Add U-Boot */ >> + uboot_start = (gd->start_addr_sp - uboot_stack_size) & >> + ~EFI_PAGE_MASK; >> + uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; >> + efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, >> + false); > > I guess it's ok to hide U-Boot from the payload for sandbox, yes. Please > extract the code into a function though, so that we don't get into too > much indenting. > OK >> + >> + /* Add Runtime Services */ >> + runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; >> + runtime_end = (ulong)&__efi_runtime_stop; >> + runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; >> + runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; >> + efi_add_memory_map(runtime_start, runtime_pages, >> + EFI_RUNTIME_SERVICES_CODE, false); >> + } > > I guess we do want to indicate RTS, no? But like everything else we want > to expose it with the real VA addresses. I suspect it would never be used but also that we should indicate RTS is present so that things that check for it don't fail. What do you think we should do here? Regards, Simon
On 12.06.18 15:48, Simon Glass wrote: > Hi Alex, > > On 12 June 2018 at 02:05, Alexander Graf <agraf@suse.de> wrote: >> >> >> On 12.06.18 07:26, Simon Glass wrote: >>> With sandbox the U-Boot code is not mapped into the sandbox memory range >>> so does not need to be excluded when allocating EFI memory. Update the EFI >>> memory init code to take account of that. >>> >>> Also use mapmem instead of a cast to convert a memory address to a >>> pointer. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >>> >>> Changes in v5: None >>> Changes in v4: None >>> Changes in v3: None >>> Changes in v2: >>> - Update to use mapmem instead of a cast >>> >>> lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- >>> 1 file changed, 18 insertions(+), 13 deletions(-) >>> >>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >>> index ec66af98ea..210e49ee8b 100644 >>> --- a/lib/efi_loader/efi_memory.c >>> +++ b/lib/efi_loader/efi_memory.c >>> @@ -9,6 +9,7 @@ >>> #include <efi_loader.h> >>> #include <inttypes.h> >>> #include <malloc.h> >>> +#include <mapmem.h> >>> #include <watchdog.h> >>> #include <linux/list_sort.h> >>> >>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) >>> &t); >>> >>> if (r == EFI_SUCCESS) { >>> - struct efi_pool_allocation *alloc = (void *)(uintptr_t)t; >>> + struct efi_pool_allocation *alloc = map_sysmem(t, size); >> >> We want to eventually be able to run efi binaries inside sandbox, so we >> need to expose a 1:1 memory map inside the efi interfaces. >> >> That means the memory argument of efi_allocate_pages() already needs to >> be set to the virtual address in real VA space. The easiest way to get >> there is if you just put virtual addresses in the efi memory map. > > Can you please explain that a bit more, or give a code example? I'm > not quite sure what you mean. In efi_add_known_memory() we populate the EFI memory table with addresses that EFI allocations can return. Because we don't control the payloads that call these functions, we can only return pointers. That means efi_add_known_memory() should add map_sysmem()'ed values into its own table. Basically while we expose "virtual 0 offset" addresses to the command line, anything internal still works on pointers. And everything EFI internal needs to be considered a pointer, because we don't control the code that deals with them. So in a nutshell, I mean something like this (untested, probably whitespace broken and line wrapped): diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ec66af98ea..80211d8c95 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -491,6 +491,9 @@ __weak void efi_add_known_memory(void) u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT; + /* Sandbox needs virtual addresses here */ + start = (uintptr_t)map_sysmem(start, pages * EFI_PAGE_SIZE); + efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY, false); } > >> >>> alloc->num_pages = num_pages; >>> *buffer = alloc->data; >>> } >>> @@ -504,18 +505,22 @@ int efi_memory_init(void) >>> >>> efi_add_known_memory(); >>> >>> - /* Add U-Boot */ >>> - uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; >>> - uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; >>> - efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false); >>> - >>> - /* Add Runtime Services */ >>> - runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; >>> - runtime_end = (ulong)&__efi_runtime_stop; >>> - runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; >>> - runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; >>> - efi_add_memory_map(runtime_start, runtime_pages, >>> - EFI_RUNTIME_SERVICES_CODE, false); >>> + if (!IS_ENABLED(CONFIG_SANDBOX)) { >>> + /* Add U-Boot */ >>> + uboot_start = (gd->start_addr_sp - uboot_stack_size) & >>> + ~EFI_PAGE_MASK; >>> + uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; >>> + efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, >>> + false); >> >> I guess it's ok to hide U-Boot from the payload for sandbox, yes. Please >> extract the code into a function though, so that we don't get into too >> much indenting. >> > > OK > >>> + >>> + /* Add Runtime Services */ >>> + runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; >>> + runtime_end = (ulong)&__efi_runtime_stop; >>> + runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; >>> + runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; >>> + efi_add_memory_map(runtime_start, runtime_pages, >>> + EFI_RUNTIME_SERVICES_CODE, false); >>> + } >> >> I guess we do want to indicate RTS, no? But like everything else we want >> to expose it with the real VA addresses. > > I suspect it would never be used but also that we should indicate RTS > is present so that things that check for it don't fail. What do you > think we should do here? I'm not 100% sure yet. It'll be very hard to support anything that relies on exit_boot_services() from within user space. So yes, just leave it out for sandbox for now. Alex
Hi Alex, On 12 June 2018 at 08:02, Alexander Graf <agraf@suse.de> wrote: > > > On 12.06.18 15:48, Simon Glass wrote: >> Hi Alex, >> >> On 12 June 2018 at 02:05, Alexander Graf <agraf@suse.de> wrote: >>> >>> >>> On 12.06.18 07:26, Simon Glass wrote: >>>> With sandbox the U-Boot code is not mapped into the sandbox memory range >>>> so does not need to be excluded when allocating EFI memory. Update the EFI >>>> memory init code to take account of that. >>>> >>>> Also use mapmem instead of a cast to convert a memory address to a >>>> pointer. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> --- >>>> >>>> Changes in v5: None >>>> Changes in v4: None >>>> Changes in v3: None >>>> Changes in v2: >>>> - Update to use mapmem instead of a cast >>>> >>>> lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- >>>> 1 file changed, 18 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >>>> index ec66af98ea..210e49ee8b 100644 >>>> --- a/lib/efi_loader/efi_memory.c >>>> +++ b/lib/efi_loader/efi_memory.c >>>> @@ -9,6 +9,7 @@ >>>> #include <efi_loader.h> >>>> #include <inttypes.h> >>>> #include <malloc.h> >>>> +#include <mapmem.h> >>>> #include <watchdog.h> >>>> #include <linux/list_sort.h> >>>> >>>> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) >>>> &t); >>>> >>>> if (r == EFI_SUCCESS) { >>>> - struct efi_pool_allocation *alloc = (void *)(uintptr_t)t; >>>> + struct efi_pool_allocation *alloc = map_sysmem(t, size); >>> >>> We want to eventually be able to run efi binaries inside sandbox, so we >>> need to expose a 1:1 memory map inside the efi interfaces. >>> >>> That means the memory argument of efi_allocate_pages() already needs to >>> be set to the virtual address in real VA space. The easiest way to get >>> there is if you just put virtual addresses in the efi memory map. >> >> Can you please explain that a bit more, or give a code example? I'm >> not quite sure what you mean. > > In efi_add_known_memory() we populate the EFI memory table with > addresses that EFI allocations can return. Because we don't control the > payloads that call these functions, we can only return pointers. That > means efi_add_known_memory() should add map_sysmem()'ed values into its > own table. > > Basically while we expose "virtual 0 offset" addresses to the command > line, anything internal still works on pointers. And everything EFI > internal needs to be considered a pointer, because we don't control the > code that deals with them. > > So in a nutshell, I mean something like this (untested, probably > whitespace broken and line wrapped): > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c > index ec66af98ea..80211d8c95 100644 > --- a/lib/efi_loader/efi_memory.c > +++ b/lib/efi_loader/efi_memory.c > @@ -491,6 +491,9 @@ __weak void efi_add_known_memory(void) > u64 start = (ram_start + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; > u64 pages = (ram_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT; > > + /* Sandbox needs virtual addresses here */ > + start = (uintptr_t)map_sysmem(start, pages * EFI_PAGE_SIZE); > + > efi_add_memory_map(start, pages, EFI_CONVENTIONAL_MEMORY, > false); > } > The map_sysmem() call is already when allocated addresses are returned - see elsewhere in the file. So adding it here as well will cause a double translation. Still, I tried this out and it just fails to init. Does any EFI app have access to the internal tables containing the memory addresses? If not, then perhaps it is OK to use U-Boot addresses here? In any case Heinrich has mentioned an alignment problem that needs to be resolved. Regards, Simon
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ec66af98ea..210e49ee8b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -9,6 +9,7 @@ #include <efi_loader.h> #include <inttypes.h> #include <malloc.h> +#include <mapmem.h> #include <watchdog.h> #include <linux/list_sort.h> @@ -393,7 +394,7 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer) &t); if (r == EFI_SUCCESS) { - struct efi_pool_allocation *alloc = (void *)(uintptr_t)t; + struct efi_pool_allocation *alloc = map_sysmem(t, size); alloc->num_pages = num_pages; *buffer = alloc->data; } @@ -504,18 +505,22 @@ int efi_memory_init(void) efi_add_known_memory(); - /* Add U-Boot */ - uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK; - uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; - efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false); - - /* Add Runtime Services */ - runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; - runtime_end = (ulong)&__efi_runtime_stop; - runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; - runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; - efi_add_memory_map(runtime_start, runtime_pages, - EFI_RUNTIME_SERVICES_CODE, false); + if (!IS_ENABLED(CONFIG_SANDBOX)) { + /* Add U-Boot */ + uboot_start = (gd->start_addr_sp - uboot_stack_size) & + ~EFI_PAGE_MASK; + uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT; + efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, + false); + + /* Add Runtime Services */ + runtime_start = (ulong)&__efi_runtime_start & ~EFI_PAGE_MASK; + runtime_end = (ulong)&__efi_runtime_stop; + runtime_end = (runtime_end + EFI_PAGE_MASK) & ~EFI_PAGE_MASK; + runtime_pages = (runtime_end - runtime_start) >> EFI_PAGE_SHIFT; + efi_add_memory_map(runtime_start, runtime_pages, + EFI_RUNTIME_SERVICES_CODE, false); + } #ifdef CONFIG_EFI_LOADER_BOUNCE_BUFFER /* Request a 32bit 64MB bounce buffer region */
With sandbox the U-Boot code is not mapped into the sandbox memory range so does not need to be excluded when allocating EFI memory. Update the EFI memory init code to take account of that. Also use mapmem instead of a cast to convert a memory address to a pointer. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: - Update to use mapmem instead of a cast lib/efi_loader/efi_memory.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)