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

[Ingo Molnar]

* Baoquan He <bhe@xxxxxxxxxx> 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