| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20130830125323.B5AFF3E102A@localhost>
Date: Fri, 30 Aug 2013 13:53:23 +0100
From: Grant Likely <grant.likely@...retlab.ca>
To: Roy Franz <roy.franz@...aro.org>, linux-kernel@...r.kernel.org,
linux-efi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
matt.fleming@...el.com, linux@....linux.org.uk
Cc: Roy Franz <roy.franz@...aro.org>, dave.martin@....com,
leif.lindholm@...aro.org, msalter@...hat.com
Subject: Re: [PATCH 02/16] Add system pointer argument to shared EFI stub related functions so they no longer use global system table pointer as they did when part of eboot.c. This code is now shared, so using a global variable as part of the interface is not that nice. Also, by avoiding any global variables in the ARM EFI stub, this allows the code to be position independent without requiring GOT fixups.
On Fri, 9 Aug 2013 16:26:03 -0700, Roy Franz <roy.franz@...aro.org> wrote:
> Signed-off-by: Roy Franz <roy.franz@...aro.org>
The entire commit message is contained in the subject line! The first
line of a commit message needs to be a short summary, followed by a
blank line, followed by the full description. Commit text body should be
wrapped at column 72 so it looks nice in 'git log'
>From your commit message:
> Subject: Re: [PATCH 02/16] Add system pointer argument to shared EFI
> stub related functions so they no longer use global system table
> pointer as they did when part of eboot.c. This code is now shared, so
> using a global variable as part of the interface is not that nice.
> Also, by avoiding any global variables in the ARM EFI stub, this
> allows the code to be position independent without requiring GOT
> fixups.
The argument about avoiding GOT fixups is stronger than the global
variables comment. I don't see a problem at all with using a global
context pointer since this code will only ever run within the context of
the EFI environment where there must always be a system table pointer.
It isn't as if there will ever be multiple system tables.
How much of a concern are GOT fixups at this stage? I thought GOT fixups
were a solved problem for the EFI stub.
g.
> ---
> arch/x86/boot/compressed/eboot.c | 38 +++++++------
> drivers/firmware/efi/efi-stub-helper.c | 96 +++++++++++++++++---------------
> 2 files changed, 72 insertions(+), 62 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index ab0eefc..65b6a34 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -453,13 +453,13 @@ struct boot_params *make_boot_params(void *handle, efi_system_table_t *_table)
> status = efi_call_phys3(sys_table->boottime->handle_protocol,
> handle, &proto, (void *)&image);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to get handle for LOADED_IMAGE_PROTOCOL\n");
> + efi_printk(sys_table, "Failed to get handle for LOADED_IMAGE_PROTOCOL\n");
> return NULL;
> }
>
> - status = low_alloc(0x4000, 1, (unsigned long *)&boot_params);
> + status = low_alloc(sys_table, 0x4000, 1, (unsigned long *)&boot_params);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc lowmem for boot params\n");
> + efi_printk(sys_table, "Failed to alloc lowmem for boot params\n");
> return NULL;
> }
>
> @@ -503,9 +503,10 @@ struct boot_params *make_boot_params(void *handle, efi_system_table_t *_table)
>
> options_size++; /* NUL termination */
>
> - status = low_alloc(options_size, 1, &cmdline);
> + status = low_alloc(sys_table, options_size, 1,
> + &cmdline);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc mem for cmdline\n");
> + efi_printk(sys_table, "Failed to alloc mem for cmdline\n");
> goto fail;
> }
>
> @@ -529,16 +530,16 @@ struct boot_params *make_boot_params(void *handle, efi_system_table_t *_table)
>
> memset(sdt, 0, sizeof(*sdt));
>
> - status = handle_ramdisks(image, hdr);
> + status = handle_ramdisks(sys_table, image, hdr);
> if (status != EFI_SUCCESS)
> goto fail2;
>
> return boot_params;
> fail2:
> if (options_size)
> - low_free(options_size, hdr->cmd_line_ptr);
> + low_free(sys_table, options_size, hdr->cmd_line_ptr);
> fail:
> - low_free(0x4000, (unsigned long)boot_params);
> + low_free(sys_table, 0x4000, (unsigned long)boot_params);
> return NULL;
> }
>
> @@ -561,7 +562,7 @@ static efi_status_t exit_boot(struct boot_params *boot_params,
> again:
> size += sizeof(*mem_map) * 2;
> _size = size;
> - status = low_alloc(size, 1, (unsigned long *)&mem_map);
> + status = low_alloc(sys_table, size, 1, (unsigned long *)&mem_map);
> if (status != EFI_SUCCESS)
> return status;
>
> @@ -569,7 +570,7 @@ get_map:
> status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
> mem_map, &key, &desc_size, &desc_version);
> if (status == EFI_BUFFER_TOO_SMALL) {
> - low_free(_size, (unsigned long)mem_map);
> + low_free(sys_table, _size, (unsigned long)mem_map);
> goto again;
> }
>
> @@ -671,7 +672,7 @@ get_map:
> return EFI_SUCCESS;
>
> free_mem_map:
> - low_free(_size, (unsigned long)mem_map);
> + low_free(sys_table, _size, (unsigned long)mem_map);
> return status;
> }
>
> @@ -694,10 +695,10 @@ static efi_status_t relocate_kernel(struct setup_header *hdr)
> EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
> nr_pages, &start);
> if (status != EFI_SUCCESS) {
> - status = low_alloc(hdr->init_size, hdr->kernel_alignment,
> - &start);
> + status = low_alloc(sys_table, hdr->init_size,
> + hdr->kernel_alignment, &start);
> if (status != EFI_SUCCESS)
> - efi_printk("Failed to alloc mem for kernel\n");
> + efi_printk(sys_table, "Failed to alloc mem for kernel\n");
> }
>
> if (status == EFI_SUCCESS)
> @@ -737,14 +738,15 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
> EFI_LOADER_DATA, sizeof(*gdt),
> (void **)&gdt);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc mem for gdt structure\n");
> + efi_printk(sys_table, "Failed to alloc mem for gdt structure\n");
> goto fail;
> }
>
> gdt->size = 0x800;
> - status = low_alloc(gdt->size, 8, (unsigned long *)&gdt->address);
> + status = low_alloc(sys_table, gdt->size, 8,
> + (unsigned long *)&gdt->address);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc mem for gdt\n");
> + efi_printk(sys_table, "Failed to alloc mem for gdt\n");
> goto fail;
> }
>
> @@ -752,7 +754,7 @@ struct boot_params *efi_main(void *handle, efi_system_table_t *_table,
> EFI_LOADER_DATA, sizeof(*idt),
> (void **)&idt);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc mem for idt structure\n");
> + efi_printk(sys_table, "Failed to alloc mem for idt structure\n");
> goto fail;
> }
>
> diff --git a/drivers/firmware/efi/efi-stub-helper.c b/drivers/firmware/efi/efi-stub-helper.c
> index 47891bd..bd6c1a2 100644
> --- a/drivers/firmware/efi/efi-stub-helper.c
> +++ b/drivers/firmware/efi/efi-stub-helper.c
> @@ -19,15 +19,16 @@ struct initrd {
>
>
>
> -static void efi_char16_printk(efi_char16_t *str)
> +static void efi_char16_printk(efi_system_table_t *sys_table_arg,
> + efi_char16_t *str)
> {
> struct efi_simple_text_output_protocol *out;
>
> - out = (struct efi_simple_text_output_protocol *)sys_table->con_out;
> + out = (struct efi_simple_text_output_protocol *)sys_table_arg->con_out;
> efi_call_phys2(out->output_string, out, str);
> }
>
> -static void efi_printk(char *str)
> +static void efi_printk(efi_system_table_t *sys_table_arg, char *str)
> {
> char *s8;
>
> @@ -37,15 +38,17 @@ static void efi_printk(char *str)
> ch[0] = *s8;
> if (*s8 == '\n') {
> efi_char16_t nl[2] = { '\r', 0 };
> - efi_char16_printk(nl);
> + efi_char16_printk(sys_table_arg, nl);
> }
>
> - efi_char16_printk(ch);
> + efi_char16_printk(sys_table_arg, ch);
> }
> }
>
>
> -static efi_status_t __get_map(efi_memory_desc_t **map, unsigned long *map_size,
> +static efi_status_t __get_map(efi_system_table_t *sys_table_arg,
> + efi_memory_desc_t **map,
> + unsigned long *map_size,
> unsigned long *desc_size)
> {
> efi_memory_desc_t *m = NULL;
> @@ -60,20 +63,20 @@ again:
> * allocation which may be in a new descriptor region.
> */
> *map_size += sizeof(*m);
> - status = efi_call_phys3(sys_table->boottime->allocate_pool,
> + status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
> EFI_LOADER_DATA, *map_size, (void **)&m);
> if (status != EFI_SUCCESS)
> goto fail;
>
> - status = efi_call_phys5(sys_table->boottime->get_memory_map, map_size,
> - m, &key, desc_size, &desc_version);
> + status = efi_call_phys5(sys_table_arg->boottime->get_memory_map,
> + map_size, m, &key, desc_size, &desc_version);
> if (status == EFI_BUFFER_TOO_SMALL) {
> - efi_call_phys1(sys_table->boottime->free_pool, m);
> + efi_call_phys1(sys_table_arg->boottime->free_pool, m);
> goto again;
> }
>
> if (status != EFI_SUCCESS)
> - efi_call_phys1(sys_table->boottime->free_pool, m);
> + efi_call_phys1(sys_table_arg->boottime->free_pool, m);
>
> fail:
> *map = m;
> @@ -83,8 +86,9 @@ fail:
> /*
> * Allocate at the highest possible address that is not above 'max'.
> */
> -static efi_status_t high_alloc(unsigned long size, unsigned long align,
> - unsigned long *addr, unsigned long max)
> +static efi_status_t high_alloc(efi_system_table_t *sys_table_arg,
> + unsigned long size, unsigned long align,
> + unsigned long *addr, unsigned long max)
> {
> unsigned long map_size, desc_size;
> efi_memory_desc_t *map;
> @@ -93,7 +97,7 @@ static efi_status_t high_alloc(unsigned long size, unsigned long align,
> u64 max_addr = 0;
> int i;
>
> - status = __get_map(&map, &map_size, &desc_size);
> + status = __get_map(sys_table_arg, &map, &map_size, &desc_size);
> if (status != EFI_SUCCESS)
> goto fail;
>
> @@ -139,7 +143,7 @@ again:
> if (!max_addr)
> status = EFI_NOT_FOUND;
> else {
> - status = efi_call_phys4(sys_table->boottime->allocate_pages,
> + status = efi_call_phys4(sys_table_arg->boottime->allocate_pages,
> EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
> nr_pages, &max_addr);
> if (status != EFI_SUCCESS) {
> @@ -152,7 +156,7 @@ again:
> }
>
> free_pool:
> - efi_call_phys1(sys_table->boottime->free_pool, map);
> + efi_call_phys1(sys_table_arg->boottime->free_pool, map);
>
> fail:
> return status;
> @@ -161,7 +165,8 @@ fail:
> /*
> * Allocate at the lowest possible address.
> */
> -static efi_status_t low_alloc(unsigned long size, unsigned long align,
> +static efi_status_t low_alloc(efi_system_table_t *sys_table_arg,
> + unsigned long size, unsigned long align,
> unsigned long *addr)
> {
> unsigned long map_size, desc_size;
> @@ -170,7 +175,7 @@ static efi_status_t low_alloc(unsigned long size, unsigned long align,
> unsigned long nr_pages;
> int i;
>
> - status = __get_map(&map, &map_size, &desc_size);
> + status = __get_map(sys_table_arg, &map, &map_size, &desc_size);
> if (status != EFI_SUCCESS)
> goto fail;
>
> @@ -203,7 +208,7 @@ static efi_status_t low_alloc(unsigned long size, unsigned long align,
> if ((start + size) > end)
> continue;
>
> - status = efi_call_phys4(sys_table->boottime->allocate_pages,
> + status = efi_call_phys4(sys_table_arg->boottime->allocate_pages,
> EFI_ALLOCATE_ADDRESS, EFI_LOADER_DATA,
> nr_pages, &start);
> if (status == EFI_SUCCESS) {
> @@ -216,17 +221,18 @@ static efi_status_t low_alloc(unsigned long size, unsigned long align,
> status = EFI_NOT_FOUND;
>
> free_pool:
> - efi_call_phys1(sys_table->boottime->free_pool, map);
> + efi_call_phys1(sys_table_arg->boottime->free_pool, map);
> fail:
> return status;
> }
>
> -static void low_free(unsigned long size, unsigned long addr)
> +static void low_free(efi_system_table_t *sys_table_arg, unsigned long size,
> + unsigned long addr)
> {
> unsigned long nr_pages;
>
> nr_pages = round_up(size, EFI_PAGE_SIZE) / EFI_PAGE_SIZE;
> - efi_call_phys2(sys_table->boottime->free_pages, addr, nr_pages);
> + efi_call_phys2(sys_table_arg->boottime->free_pages, addr, nr_pages);
> }
>
>
> @@ -236,7 +242,8 @@ static void low_free(unsigned long size, unsigned long addr)
> * We only support loading an initrd from the same filesystem as the
> * kernel image.
> */
> -static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
> +static efi_status_t handle_ramdisks(efi_system_table_t *sys_table_arg,
> + efi_loaded_image_t *image,
> struct setup_header *hdr)
> {
> struct initrd *initrds;
> @@ -278,12 +285,12 @@ static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
> if (!nr_initrds)
> return EFI_SUCCESS;
>
> - status = efi_call_phys3(sys_table->boottime->allocate_pool,
> + status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
> EFI_LOADER_DATA,
> nr_initrds * sizeof(*initrds),
> &initrds);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc mem for initrds\n");
> + efi_printk(sys_table_arg, "Failed to alloc mem for initrds\n");
> goto fail;
> }
>
> @@ -329,18 +336,18 @@ static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
> if (!i) {
> efi_boot_services_t *boottime;
>
> - boottime = sys_table->boottime;
> + boottime = sys_table_arg->boottime;
>
> status = efi_call_phys3(boottime->handle_protocol,
> image->device_handle, &fs_proto, &io);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to handle fs_proto\n");
> + efi_printk(sys_table_arg, "Failed to handle fs_proto\n");
> goto free_initrds;
> }
>
> status = efi_call_phys2(io->open_volume, io, &fh);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to open volume\n");
> + efi_printk(sys_table_arg, "Failed to open volume\n");
> goto free_initrds;
> }
> }
> @@ -348,9 +355,9 @@ static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
> status = efi_call_phys5(fh->open, fh, &h, filename_16,
> EFI_FILE_MODE_READ, (u64)0);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to open initrd file: ");
> - efi_char16_printk(filename_16);
> - efi_printk("\n");
> + efi_printk(sys_table_arg, "Failed to open initrd file: ");
> + efi_char16_printk(sys_table_arg, filename_16);
> + efi_printk(sys_table_arg, "\n");
> goto close_handles;
> }
>
> @@ -360,30 +367,31 @@ static efi_status_t handle_ramdisks(efi_loaded_image_t *image,
> status = efi_call_phys4(h->get_info, h, &info_guid,
> &info_sz, NULL);
> if (status != EFI_BUFFER_TOO_SMALL) {
> - efi_printk("Failed to get initrd info size\n");
> + efi_printk(sys_table_arg, "Failed to get initrd info size\n");
> goto close_handles;
> }
>
> grow:
> - status = efi_call_phys3(sys_table->boottime->allocate_pool,
> + status = efi_call_phys3(sys_table_arg->boottime->allocate_pool,
> EFI_LOADER_DATA, info_sz, &info);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc mem for initrd info\n");
> + efi_printk(sys_table_arg, "Failed to alloc mem for initrd info\n");
> goto close_handles;
> }
>
> status = efi_call_phys4(h->get_info, h, &info_guid,
> &info_sz, info);
> if (status == EFI_BUFFER_TOO_SMALL) {
> - efi_call_phys1(sys_table->boottime->free_pool, info);
> + efi_call_phys1(sys_table_arg->boottime->free_pool,
> + info);
> goto grow;
> }
>
> file_sz = info->file_size;
> - efi_call_phys1(sys_table->boottime->free_pool, info);
> + efi_call_phys1(sys_table_arg->boottime->free_pool, info);
>
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to get initrd info\n");
> + efi_printk(sys_table_arg, "Failed to get initrd info\n");
> goto close_handles;
> }
>
> @@ -399,16 +407,16 @@ grow:
> * addresses in memory, so allocate enough memory for
> * all the initrd's.
> */
> - status = high_alloc(initrd_total, 0x1000,
> + status = high_alloc(sys_table_arg, initrd_total, 0x1000,
> &initrd_addr, hdr->initrd_addr_max);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to alloc highmem for initrds\n");
> + efi_printk(sys_table_arg, "Failed to alloc highmem for initrds\n");
> goto close_handles;
> }
>
> /* We've run out of free low memory. */
> if (initrd_addr > hdr->initrd_addr_max) {
> - efi_printk("We've run out of free low memory\n");
> + efi_printk(sys_table_arg, "We've run out of free low memory\n");
> status = EFI_INVALID_PARAMETER;
> goto free_initrd_total;
> }
> @@ -428,7 +436,7 @@ grow:
> initrds[j].handle,
> &chunksize, addr);
> if (status != EFI_SUCCESS) {
> - efi_printk("Failed to read initrd\n");
> + efi_printk(sys_table_arg, "Failed to read initrd\n");
> goto free_initrd_total;
> }
> addr += chunksize;
> @@ -440,7 +448,7 @@ grow:
>
> }
>
> - efi_call_phys1(sys_table->boottime->free_pool, initrds);
> + efi_call_phys1(sys_table_arg->boottime->free_pool, initrds);
>
> hdr->ramdisk_image = initrd_addr;
> hdr->ramdisk_size = initrd_total;
> @@ -448,13 +456,13 @@ grow:
> return status;
>
> free_initrd_total:
> - low_free(initrd_total, initrd_addr);
> + low_free(sys_table_arg, initrd_total, initrd_addr);
>
> close_handles:
> for (k = j; k < i; k++)
> efi_call_phys1(fh->close, initrds[k].handle);
> free_initrds:
> - efi_call_phys1(sys_table->boottime->free_pool, initrds);
> + efi_call_phys1(sys_table_arg->boottime->free_pool, initrds);
> fail:
> hdr->ramdisk_image = 0;
> hdr->ramdisk_size = 0;
> --
> 1.7.10.4
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists