linux-kernel - Re: [PATCH v6 RESEND] x86/boot/KASLR: Restrict kernel to be randomized in mirror regions

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: <20170721103757.hc74czr3mfunrv6c@gmail.com>
Date:   Fri, 21 Jul 2017 12:37:57 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Baoquan He <bhe@...hat.com>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        keescook@...omium.org, matt@...eblueprint.co.uk,
        tglx@...utronix.de, hpa@...or.com, izumi.taku@...fujitsu.com,
        fanc.fnst@...fujitsu.com, thgarnie@...gle.com,
        n-horiguchi@...jp.nec.com
Subject: Re: [PATCH v6 RESEND] x86/boot/KASLR: Restrict kernel to be
 randomized in mirror regions


* Baoquan He <bhe@...hat.com> wrote:

> +/*
> + * Returns true if mirror region found (and must have been processed
> + * for slots adding)
> + */
> +static bool process_efi_entries(unsigned long minimum,
> +				unsigned long image_size)

Also, please don't break the line in the middle of the prototype.

> +{
> +	struct efi_info *e = &boot_params->efi_info;
> +	bool efi_mirror_found = false;
> +	struct mem_vector region;
> +	efi_memory_desc_t *md;
> +	unsigned long pmap;
> +	char *signature;
> +	u32 nr_desc;
> +	int i;
> +
> +	signature = (char *)&boot_params->efi_info.efi_loader_signature;

This is sloppy too: we already have '&boot_params->efi_info' in 'e', why do you 
duplicate it again, why not write 'e->efi_loader_signature'??

> +	if (strncmp(signature, EFI32_LOADER_SIGNATURE, 4) &&
> +	    strncmp(signature, EFI64_LOADER_SIGNATURE, 4))
> +		return false;
> +
> +#ifdef CONFIG_X86_32
> +	/* Can't handle data above 4GB at this time */
> +	if (e->efi_memmap_hi) {
> +		warn("Memory map is above 4GB, EFI should be disabled.\n");
> +		return false;

This kernel warning is pretty passive-aggressive: please explain what the problem 
is and how it can be resolved.

> +	}
> +	pmap =  e->efi_memmap;
> +#else
> +	pmap = (e->efi_memmap | ((__u64)e->efi_memmap_hi << 32));
> +#endif
> +
> +	nr_desc = e->efi_memmap_size / e->efi_memdesc_size;
> +	for (i = 0; i < nr_desc; i++) {
> +		md = (efi_memory_desc_t *)(pmap + (i * e->efi_memdesc_size));

This looks unnecessarily obfuscated: why not initialize 'md' to 'pmap' when pmap 
is calculated and just use md[i]?

> +static inline bool process_efi_entries(unsigned long minimum,
> +				       unsigned long image_size)

ugly linebreak again ...

Thanks,

	Ingo