linux-kernel - Re: [PATCH 1/2] x86/boot: robustify calling startup

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0fcbedf6-3eb5-75cd-cdd9-24582f70cc64@kernel.org>
Date:   Tue, 1 Nov 2022 07:47:49 +0100
From:   Jiri Slaby <jirislaby@...nel.org>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Cc:     "H. Peter Anvin" <hpa@...or.com>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Tony Luck <tony.luck@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Masahiro Yamada <masahiroy@...nel.org>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] x86/boot: robustify calling startup_{32,64}() from
 the decompressor code

On 31. 10. 22, 16:10, Alexander Lobakin wrote:
> After commit ce697ccee1a8 ("kbuild: remove head-y syntax"), I
> started digging whether x86 is ready from removing this old cruft.
> Removing its objects from the list makes the kernel unbootable.
> This applies only to bzImage, vmlinux still works correctly.
> The reason is that with no strict object order determined by the
> linker arguments, not the linker script, startup_64 can be placed
> not right at the beginning of the kernel.
> Here's vmlinux.map's beginning before removing:
> 
> ffffffff81000000         vmlinux.o:(.head.text)
> ffffffff81000000                 startup_64
> ffffffff81000070                 secondary_startup_64
> ffffffff81000075                 secondary_startup_64_no_verify
> ffffffff81000160                 verify_cpu
> 
> and after:
> 
> ffffffff81000000         vmlinux.o:(.head.text)
> ffffffff81000000                 pvh_start_xen
> ffffffff81000080                 startup_64
> ffffffff810000f0                 secondary_startup_64
> ffffffff810000f5                 secondary_startup_64_no_verify
> 
> Not a problem itself, but the self-extractor code has the address of
> that function hardcoded the beginning, not looking onto the ELF
> header, which always contains the address of startup_{32,64}().
> 
> So, instead of doing an "act of blind faith", just take the address
> from the ELF header and extract a relative offset to the entry
> point. The decompressor function already returns a pointer to the
> beginning of the kernel to the Asm code, which then jumps to it,
> so add that offset to the return value.
> This doesn't change anything for now, but allows to resign from the
> "head object list" for x86 and makes sure valid Kbuild or any other
> improvements won't break anything here in general.

Oh yeah! I wouldn't think that implementing this would be _that_ easy.

The next natural step would be to eliminate the whole head section. But 
that would need a bit more work as not all jumps are rip-relative, 
apparently...

Few comments below, so no Reviewed-by yet.

Tested-by: Jiri Slaby <jirislaby@...nel.org>

> Signed-off-by: Alexander Lobakin <alexandr.lobakin@...el.com>
...
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -277,7 +277,7 @@ static inline void handle_relocations(void *output, unsigned long output_len,
>   { }
>   #endif
>   
> -static void parse_elf(void *output)
> +static size_t parse_elf(void *output)
>   {
>   #ifdef CONFIG_X86_64
>   	Elf64_Ehdr ehdr;
> @@ -287,6 +287,7 @@ static void parse_elf(void *output)
>   	Elf32_Phdr *phdrs, *phdr;
>   #endif
>   	void *dest;
> +	size_t off;
>   	int i;
>   
>   	memcpy(&ehdr, output, sizeof(ehdr));
> @@ -295,16 +296,19 @@ static void parse_elf(void *output)
>   	   ehdr.e_ident[EI_MAG2] != ELFMAG2 ||
>   	   ehdr.e_ident[EI_MAG3] != ELFMAG3) {
>   		error("Kernel is not a valid ELF file");
> -		return;
> +		return 0;

error() is noreturn, so you can remove these returns. They don't make 
sense anyway. Likely in a separate patch.

>   	}
>   
>   	debug_putstr("Parsing ELF... ");
>   
>   	phdrs = malloc(sizeof(*phdrs) * ehdr.e_phnum);
> -	if (!phdrs)
> +	if (!phdrs) {
>   		error("Failed to allocate space for phdrs");
> +		return 0;
> +	}
>   
>   	memcpy(phdrs, output + ehdr.e_phoff, sizeof(*phdrs) * ehdr.e_phnum);
> +	off = ehdr.e_entry - phdrs->p_paddr;
>   
>   	for (i = 0; i < ehdr.e_phnum; i++) {
>   		phdr = &phdrs[i];
> @@ -328,6 +332,7 @@ static void parse_elf(void *output)
>   	}
>   
>   	free(phdrs);
> +	return off;

You should add a \n before the return.

>   }
>   
>   /*
> @@ -356,6 +361,7 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>   	const unsigned long kernel_total_size = VO__end - VO__text;
>   	unsigned long virt_addr = LOAD_PHYSICAL_ADDR;
>   	unsigned long needed_size;
> +	size_t off;
>   
>   	/* Retain x86 boot parameters pointer passed from startup_32/64. */
>   	boot_params = rmode;
> @@ -456,14 +462,14 @@ asmlinkage __visible void *extract_kernel(void *rmode, memptr heap,
>   	debug_putstr("\nDecompressing Linux... ");
>   	__decompress(input_data, input_len, NULL, NULL, output, output_len,
>   			NULL, error);
> -	parse_elf(output);
> +	off = parse_elf(output);

Perhaps add:
   debug_putaddr(off);
here?

>   	handle_relocations(output, output_len, virt_addr);
>   	debug_putstr("done.\nBooting the kernel.\n");

>   
>   	/* Disable exception handling before booting the kernel */
>   	cleanup_exception_handling();
>   
> -	return output;
> +	return output + off;
>   }
>   
>   void fortify_panic(const char *name)

thanks,
-- 
js
suse labs