DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7E8CD85550
Date: Fri, 21 Jul 2017 21:19:56 +0800
From: Baoquan He <bhe@redhat.com>
To: Ingo Molnar <mingo@kernel.org>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org, keescook@chromium.org,
        matt@codeblueprint.co.uk, tglx@linutronix.de, hpa@zytor.com,
        izumi.taku@jp.fujitsu.com, fanc.fnst@cn.fujitsu.com,
        thgarnie@google.com, n-horiguchi@ah.jp.nec.com
Subject: Re: [PATCH v6 RESEND] x86/boot/KASLR: Restrict kernel to be
 randomized in mirror regions
Message-ID: <20170721131956.GK2344@x1>
References: <1500542189-15779-1-git-send-email-bhe@redhat.com>
 <20170721103757.hc74czr3mfunrv6c@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170721103757.hc74czr3mfunrv6c@gmail.com>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2620
Lines: 86

On 07/21/17 at 12:37pm, Ingo Molnar wrote:
> 
> * Baoquan He <bhe@redhat.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.

OK, will change it into oneline. I worry it's a little too long since
it's 80 chars wide if not break.
> 
> > +{
> > +	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'??

Right, will change.

> 
> > +	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.
Maybe it can be like :

warn("Data above 4G can't be handled now in 32bit system, EFI should be disabled.\n");
> > +		return false;

> 
> > +	}
> > +	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]?

There are places where the efi map is getting and used like this. E.g
in efi_high_alloc() of drivers/firmware/efi/libstub/efi-stub-helper.c.
EFI developers worry the size of efi_memory_desc_t could not be the same
as e->efi_memdesc_size?

Hi Matt,

Could you help have a look at this?

> 
> > +static inline bool process_efi_entries(unsigned long minimum,
> > +				       unsigned long image_size)
> 
> ugly linebreak again ...

The whole line is more than 80. I break the line and use tab and space
to make it align with above 'unsigned long minimum'. Don't know why it
becomes messy in patch. Will check and try again.

Thanks
Baoquan