lkml.org 
[lkml]   [2015]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware
On Mon, Oct 5, 2015 at 9:07 AM, Kweh, Hock Leong
<hock.leong.kweh@intel.com> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto@amacapital.net]
>> Sent: Sunday, October 04, 2015 7:16 AM
>> > +
>> > + /* setup capsule binary info structure */
>> > + if (cap_info.header_obtained == 0 && cap_info.index == 0) {
>> > + efi_capsule_header_t *cap_hdr = kbuff;
>> > + int reset_type;
>> > + size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
>> > + PAGE_SHIFT;
>> > +
>> > + if (pages_needed == 0) {
>> > + pr_err("%s: pages count invalid\n", __func__);
>> > + kunmap(kbuff_page);
>> > + efi_free_all_buff_pages(kbuff_page);
>> > + return -EINVAL;
>> > + }
>> > +
>> > + /* check if the capsule binary supported */
>> > + ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
>> > + cap_hdr->imagesize,
>> > + &reset_type);
>>
>> And what if cap_hdr isn't written yet?
>
> This design mainly targeting a simplest interface that user could upload efi
> capsule in a single command action: cat capsule.bin > /dev/efi_capsule_loader
>
> So, it is expected that efi capsule header is at the starting of the binary file.
> Already capture this into efi_capsule_write() comment in v7 patchset:
> https://lkml.org/lkml/2015/10/5/232
>
> If you want to enhance this module to support creating efi capsule header for
> your binary, strongly believe this design can cater the implementation such as
> adding ioctl to pass in efi guid, flags and so on parameters to create the header.
>

No, that's not what I mean. What I mean is: what if cat writes too
little in the first write call (e.g. 3 bytes).

>
>>
>> > + if (ret) {
>> > + pr_err("%s: efi_capsule_supported() failed\n",
>> > + __func__);
>> > + kunmap(kbuff_page);
>> > + efi_free_all_buff_pages(kbuff_page);
>> > + return ret;
>> > + }
>> > +
>> > + cap_info.total_size = cap_hdr->imagesize;
>> > + cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
>> > + GFP_KERNEL);
>> > + if (!cap_info.pages) {
>> > + pr_debug("%s: kmalloc_array() failed\n", __func__);
>> > + kunmap(kbuff_page);
>> > + efi_free_all_buff_pages(kbuff_page);
>> > + return -ENOMEM;
>> > + }
>> > +
>> > + cap_info.header_obtained = 1;
>>
>> I don't see how you know that the header is obtained.
>
> Capsule header is at the starting block of image binary. We can
> obtain the header through the 1st block of write action.

That's quite an assumption to make.

> So,
> user app is expected to upload the image binary sequentially.
>

>> > + cap_info.pages[cap_info.index++] = kbuff_page;
>>
>> Huh? You might now have allocated a whole page.
>
> Yes, the efi capsule header does tell the whole image size.

So what? Did you allocate a page in this particular write call? If
so, then cap_info.index++, etc is okay. If not, it's wrong.

>> > + cap_info.count += write_byte;
>> > + kunmap(kbuff_page);
>> > +
>> > + /* submit the full binary to efi_capsule_update() API */
>> > + if (cap_info.count >= cap_info.total_size) {
>> > + void *cap_hdr_temp;
>> > +
>> > + cap_hdr_temp = kmap(cap_info.pages[0]);
>> > + if (!cap_hdr_temp) {
>> > + pr_debug("%s: kmap() failed\n", __func__);
>> > + efi_free_all_buff_pages(NULL);
>> > + return -EFAULT;
>> > + }
>> > + ret = efi_capsule_update(cap_hdr_temp, cap_info.pages);
>> > + kunmap(cap_info.pages[0]);
>> > + if (ret) {
>> > + pr_err("%s: efi_capsule_update() failed\n", __func__);
>> > + efi_free_all_buff_pages(NULL);
>> > + return ret;
>> > + }
>> > + /* indicate capsule binary uploading is done */
>> > + cap_info.index = -1;
>>
>> Should count > cap_info.total_size be an error?
>>
>> --Andy
>
> Yes, this is why after the write count already reaches the image size stated in
> efi capsule header, an indicator will be flagged for subsequence write to be
> returned -EIO as what Matt has commented.

What if *this very same write* writes too much data?

--Andy


\
 
 \ /
  Last update: 2015-10-05 21:21    [W:0.048 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site