Re: [PATCH v7 2/8] efi: Add embedded peripheral firmware support

[Luis Chamberlain <mcgrof@kernel.org>]

On Thu, Nov 14, 2019 at 12:27:01PM +0100, Hans de Goede wrote:
> Hi Luis,
> 
> Thank you for the reviews and sorry for being a bit slow to respind.
> 
> On 11-10-2019 16:48, Luis Chamberlain wrote:
> > On Fri, Oct 04, 2019 at 04:50:50PM +0200, Hans de Goede wrote:
> > > +static int __init efi_check_md_for_embedded_firmware(
> > > +	efi_memory_desc_t *md, const struct efi_embedded_fw_desc *desc)
> > > +{
> > > +	const u64 prefix = *((u64 *)desc->prefix);
> > > +	struct sha256_state sctx;
> > > +	struct embedded_fw *fw;
> > > +	u8 sha256[32];
> > > +	u64 i, size;
> > > +	void *map;
> > > +
> > > +	size = md->num_pages << EFI_PAGE_SHIFT;
> > > +	map = memremap(md->phys_addr, size, MEMREMAP_WB);
> > 
> > Since our limitaiton is the init process must have mostly finished,
> > it implies early x86 boot code cannot use this, what measures can we
> > take to prevent / check for such conditions to be detected and
> > gracefully errored out?
> 
> As with all (EFI) early boot code, there simply is a certain order
> in which things need to be done. This needs to happen after the basic
> mm is setup, but before efi_free_boot_services() gets called, there
> isn't really a way to check for all these conditions. As with all
> early boot code, people making changes need to be careful to not
> break stuff.

I rather we take a proactive measure here and add whatever it is we need
to ensure the API works only when its supposed to, rather than try and
fail, and then expect the user to know these things.

I'd prefer if we at least try to address this.

> > > +	if (!map) {
> > > +		pr_err("Error mapping EFI mem at %#llx\n", md->phys_addr);
> > > +		return -ENOMEM;
> > > +	}
> > > +
> > > +	size -= desc->length;
> > 
> > Remind me again, why we decrement the size here?
> 
> Basically this is another way of writing:
> 
> 	for (i = 0; (i + desc->length) < size; i += 8) {
> 
> > I was going to ask if we didn't need a:
> > 
> > if (desc->length > size) {
> > 	memunmap(map);
> > 	return -EINVAL;
> > }
> 
> That is a good point, unlikely but still a good point,
> so I guess that writing:
> 
> 	for (i = 0; (i + desc->length) < size; i += 8) {
> 
> Instead would better as that avoids the need for that check.
> I will fix this for the next version.

Great thanks.

  Luis