Message-ID: <F54AEECA5E2B9541821D670476DAE19C4A8ABEFB@PGSMSX102.gar.corp.intel.com>
Date:	Tue, 26 Jan 2016 03:09:42 +0000
From:	"Kweh, Hock Leong" <hock.leong.kweh@...el.com>
To:	'Bryan O'Donoghue' <pure.logic@...us-software.ie>,
	Matt Fleming <matt@...sole-pimps.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Matt Fleming <matt@...eblueprint.co.uk>
CC:	"Ong, Boon Leong" <boon.leong.ong@...el.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
	Sam Protsenko <semen.protsenko@...aro.org>,
	Peter Jones <pjones@...hat.com>,
	Andy Lutomirski <luto@...capital.net>,
	"Roy Franz" <roy.franz@...aro.org>, Borislav Petkov <bp@...en8.de>,
	James Bottomley <James.Bottomley@...senpartnership.com>,
	Linux FS Devel <linux-fsdevel@...r.kernel.org>,
	"Anvin, H Peter" <h.peter.anvin@...el.com>
Subject: RE: [PATCH v10 1/1] efi: a misc char interface for user to update
 efi firmware

> -----Original Message-----
> From: Bryan O'Donoghue [mailto:pure.logic@...us-software.ie]
> Sent: Tuesday, December 22, 2015 1:04 AM
> 
> Small nit.
> 

Hi, Sorry for the late response. Happy New Year.

> On Fri, 2015-12-18 at 20:13 +0800, Kweh, Hock Leong wrote:
> > From: "Kweh, Hock Leong" <hock.leong.kweh@...el.com>
> >
> > Introducing a kernel module to expose capsule loader interface
> > (misc char device file note) for user to upload capsule binaries.
> 
> This patch ? introduces a kernel module to expose a capsule loader
> interface... for a user ...
> 
> >
> > Example method to load the capsule binary:
> 
> Example:
> 

Noted.

> > cat firmware.bin > /dev/efi_capsule_loader
> >
> > This patch also export efi_capsule_supported() function symbol for
> > verifying the submitted capsule header in this kernel module.
> 
> 1. Should be a separate patch
> 2. Suggested, rewording for that patch log
> 
> 2/2 Title
> This patch exports efi_capsule_supported to enable verification of the
> capsule header.
> 
> That said - who is the user of this symbol ? Why is it needed ? Anyway
> please consider splitting and rewording.
> 

Answered by Borislav.

> >
> > +config EFI_CAPSULE_LOADER
> > +	tristate "EFI capsule loader"
> > +	depends on EFI
> > +	help
> > +	  This option exposes a loader interface
> > "/dev/efi_capsule_loader" for
> > +	  user to load EFI capsule binary and update the EFI
> > firmware. After
> > +	  a successful loading, a system reboot is required.
> > +
> > +	  If unsure, say N.
> > +
> 
> This option exposes a loader interface... for a user to load an EFI
> capsule.
> 
> A capsule isn't necessarily exclusively for updating of firmware either
> AFAIK - so I suggest dropping.
> 
> http://www.intel.com/content/www/us/en/architecture-and-
> technology/unif
> ied-extensible-firmware-interface/efi-capsule-specification.html
> 
> Quote "Capsules were designed for and are intended to be the major
> vehicle for delivering firmware volume changes. There is no requirement
> that capsules be limited to that function."
> 

Noted.

> >
> > +
> > +/**
> > + * efi_free_all_buff_pages - free all previous allocated buffer
> > pages
> > + * @cap_info: pointer to current instance of capsule_info structure
> > + *
> > + *	Besides freeing the buffer pages, it also flagged an
> > + *	NO_FURTHER_WRITE_ACTION flag for indicating to the next
> > write entries
> > + *	that there is a flaw happened and any subsequence actions
> > are not
> > + *	valid unless close(2).
> > + **/
> 
> The description needs to be reworded.
> 
> In addition to freeing buffer pages, we flag NO_FURTHER_WRITE_ACTION on
> error and will cease to process data in subsequent efi_capsule_write()
> calls.
> 
> (or similar)
> 

Noted.

> > +
> > +/**
> > + * efi_capsule_setup_info - to obtain the efi capsule header in the
> > binary and
> > + *			    setup capsule_info structure
> > + * @cap_info: pointer to current instance of capsule_info structure
> > + * @kbuff: a mapped 1st page buffer pointer
> 
> first not 1st
> 

Noted.

> > + * @hdr_bytes: the total bytes of received efi header
> 
> the total number of received bytes of the EFI header
> 

Noted.

> > + **/
> > +static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
> > +				      void *kbuff, size_t hdr_bytes)
> > +{
> > +	int ret;
> > +	void *temp_page;
> > +
> > +	/* Handling 1st block is less than header size */
> first or initial
> I think you mean to say "Ensure first
> 

No. I mean handling. I will change to "first"

> > +
> > +		/* Check if the capsule binary supported */
> > +		mutex_lock(&capsule_loader_lock);
> > +		ret = efi_capsule_supported(cap_hdr->guid, cap_hdr
> > ->flags,
> > +					    cap_hdr->imagesize,
> > +					    &cap_info->reset_type);
> > +		mutex_unlock(&capsule_loader_lock);
> 
> What does this mutex really do ? You hold it here around
> efi_capsule_supported() in the call
> 
> chain efi_capsule_write() => efi_capsule_setup_info()
> 
> and then again inside of efi_capsule_submit_update() the call chain
> 
> chain efi_capsule_write() => efi_capsule_submit_update()
> 
> So if its valid to ensure you are synchronizing parallel contexts with
> -respect to calls into efi_capsule_supported() and
> efi_capsule_submit_update() why aren't you holding that lock for the
> duration of efi_capsule_write() - couldn't cap_info->page_bytes_remain
> as an example equally be racy ?
> 

This is to protect multiple instant calling open(2) and still able to synchronize
the calling to efi_capsule_supported() and efi_capsule_update() when they
are uploading the capsule concurrently.

> 
> > +
> > +/**
> > + * efi_capsule_submit_update - invoke the efi_capsule_update API
> > once binary
> > + *			       upload done
> > + * @cap_info: pointer to current instance of capsule_info structure
> > + **/
> > +static ssize_t efi_capsule_submit_update(struct capsule_info
> > *cap_info)
> > +{
> > +	int ret;
> > +	void *cap_hdr_temp;
> > +
> > +	cap_hdr_temp = kmap(cap_info->pages[0]);
> > +	if (!cap_hdr_temp) {
> > +		pr_debug("%s: kmap() failed\n", __func__);
> > +		return -EFAULT;
> > +	}
> > +
> > +	mutex_lock(&capsule_loader_lock);
> > +	ret = efi_capsule_update(cap_hdr_temp, cap_info->pages);
> > +	mutex_unlock(&capsule_loader_lock);
> > +	kunmap(cap_info->pages[0]);
> > +	if (ret) {
> > +		pr_err("%s: efi_capsule_update() failed\n",
> > __func__);
> > +		return ret;
> > +	}
> > +
> > +	/* Indicate capsule binary uploading is done */
> > +	cap_info->index = NO_FURTHER_WRITE_ACTION;
> 
> Again - if you need a mutex for efi_capsule_update() why don't you need
> a mutex for cap_info->index updates looks racy...
> 

Answered as above.

> +}
> > +
> > +/**
> > + * efi_capsule_write - store the capsule binary and pass it to
> > + *		       efi_capsule_update() API
> > + * @file: file pointer
> > + * @buff: buffer pointer
> > + * @count: number of bytes in @buff
> > + * @offp: not used
> > + *
> > + *	Expectation:
> > + *	- User space tool should start at the beginning of capsule
> > binary and
> > + *	  pass data in sequential.
> 
> A user-space tool.. sequentially
> 

Noted.

> > + *	- User should close and re-open this file note in order to
> > upload more
> > + *	  capsules.
> 
> A user should..
> 

Will change to use plural.

> > + *	- Any error occurred, user should close the file and
> > restart the
> > + *	  operation for the next try otherwise -EIO will be
> > returned until the
> > + *	  file is closed.
> 
> On error -EIO will be returned and capsule loading will be abandoned.
> 

What I am trying to tell is that after an error happen (for e.g. -ENOMEM),
then user should close it and restart the file open, write & close operation.
If not, then only -EIO will be returned.

I do not means that any error happen, only -EIO is returned.

> 
> > + *	- EFI capsule header must be located at the beginning of
> > capsule binary
> > + *	  file and passed in as 1st block write.
> 
> An EFI capsule header.... in as the first data in the first write
> operation
> 

Noted.

> 
> > + **/
> > +static ssize_t efi_capsule_write(struct file *file, const char
> > __user *buff,
> > +				 size_t count, loff_t *offp)
> > +{
> > +	int ret = 0;
> > +	struct capsule_info *cap_info = file->private_data;
> > +	struct page *kbuff_page = NULL;
> > +	void *kbuff = NULL;
> > +	size_t write_byte;
> > +
> > +	if (count == 0)
> > +		return 0;
> > +
> > +	/* Return error while NO_FURTHER_WRITE_ACTION is flagged */
> > +	if (cap_info->index < 0)
> > +		return -EIO;
> > +
> > +	/* Only alloc a new page when previous page is full */
> > +	if (!cap_info->page_bytes_remain) {
> > +		kbuff_page = alloc_page(GFP_KERNEL);
> > +		if (!kbuff_page) {
> > +			pr_debug("%s: alloc_page() failed\n",
> > __func__);
> 
> Shouldn't this be a pr_err() ?
> 

As mentioned by Borislav, error will be propagated by -ENOMEM and there is not needed
to have double printing.

> > +			ret = -ENOMEM;
> > +			goto failed;
> > +		}
> > +		cap_info->page_bytes_remain = PAGE_SIZE;
> > +	} else {
> > +		kbuff_page = cap_info->pages[--cap_info->index];
> 
> How are you guaranteeing this index doesn't go negative ? You compare
> index < 0 above so the pre-decrement could be negative ... right ?
> 

Taking care by ->page_bytes_remain, if ->index is zero then ->page_bytes_remain
must be zero.

> > +	}
> > +
> > +	kbuff = kmap(kbuff_page);
> > +	if (!kbuff) {
> > +		pr_debug("%s: kmap() failed\n", __func__);
> 
> and here ?
> 
> > +		ret = -EFAULT;
> > +		goto fail_freepage;
> > +	}
> > +	kbuff += PAGE_SIZE - cap_info->page_bytes_remain;
> > +
> > +	/* Copy capsule binary data from user space to kernel space
> > buffer */
> > +	write_byte = min_t(size_t, count, cap_info
> > ->page_bytes_remain);
> > +	if (copy_from_user(kbuff, buff, write_byte)) {
> > +		pr_debug("%s: copy_from_user() failed\n", __func__);
> 
> ditto
> 
> > +		ret = -EFAULT;
> > +		goto fail_unmap;
> > +	}
> > +	cap_info->page_bytes_remain -= write_byte;
> > +
> > +	/* Setup capsule binary info structure */
> > +	if (!cap_info->header_obtained) {
> > +		ret = efi_capsule_setup_info(cap_info, kbuff,
> > +					     cap_info->count +
> > write_byte);
> > +		if (ret)
> > +			goto fail_unmap;
> > +	}
> > +
> > +	cap_info->pages[cap_info->index++] = kbuff_page;
> > +	cap_info->count += write_byte;
> > +	kunmap(kbuff_page);
> > +
> > +	/* Submit the full binary to efi_capsule_update() API */
> > +	if (cap_info->header_obtained &&
> > +	    cap_info->count >= cap_info->total_size) {
> > +		if (cap_info->count > cap_info->total_size) {
> > +			pr_err("%s: upload size exceeded header
> > defined size\n",
> > +			       __func__);
> > +			ret = -EINVAL;
> > +			goto failed;
> 
> Shouldn't this be fail_freepage ?
> 

No, at this stage, kbuff_page no longer valid and have been assigned to
->pages[].

> > +		}
> > +
> > +		ret = efi_capsule_submit_update(cap_info);
> > +		if (ret)
> > +			goto failed;
> 
> Shouldn't this be fail_freepage ?
> 

Same as above.

> > +	}
> > +
> > +	return write_byte;
> > +
> > +fail_unmap:
> > +	kunmap(kbuff_page);
> > +fail_freepage:
> > +	__free_page(kbuff_page);
> > +failed:
> > +	efi_free_all_buff_pages(cap_info);
> > +	return ret;
> > +}
> > +
> > +/**
> > + * efi_capsule_flush - called by file close or file flush
> > + * @file: file pointer
> > + * @id: not used
> > + *
> > + *	If capsule being uploaded partially, calling this function
> > will be
> > + *	treated as uploading canceled and will free up those
> 
> 
> 
> > completed buffer
> > + *	pages and then return -ECANCELED.
> 
> If a capsule is being partially updated then calling this function will
> be treated as upload termination and will free completed buffer pages;
> in this case -ECANCELLED will be returned.
> 

Noted.

> 
> > + **/
> > +static int efi_capsule_flush(struct file *file, fl_owner_t id)
> > +{
> > +	int ret = 0;
> > +	struct capsule_info *cap_info = file->private_data;
> > +
> > +	if (cap_info->index > 0) {
> > +		pr_err("%s: capsule upload not complete\n",
> > __func__);
> > +		efi_free_all_buff_pages(cap_info);
> > +		ret = -ECANCELED;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * efi_capsule_release - called by file close
> > + * @inode: not used
> > + * @file: file pointer
> > + *
> > + *	We would not free the successful submitted buffer pages
> > here due to
> > + *	efi update require system reboot with data maintained.
> > + **/
> 
> We will not free successfully submitted pages since EFI update requires
> data to be maintained across boot.
> 

Noted.

> 
> > +static int efi_capsule_open(struct inode *inode, struct file *file)
> > +{
> > +	struct capsule_info *cap_info;
> > +
> > +	cap_info = kzalloc(sizeof(*cap_info), GFP_KERNEL);
> > +	if (!cap_info)
> > +		return -ENOMEM;
> > +
> > +	file->private_data = cap_info;
> > +
> > +	return 0;
> > +}
> 
> You have a memory leak here don't you ?
> 
> if I do
> for (i = 0; i < N; i++) {
> 	fd = open(/dev/your_node);
> 	close(fd);
> }
> 
> You'll leak that kzalloc...

When you perform close(fd), efi_capsule_release() will be called and
cap_info will be freed there.


Thanks & Regards,
Wilson

Powered by blists - more mailing lists