Re: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware

[Andy Lutomirski]

On Mon, Oct 5, 2015 at 9:07 AM, Kweh, Hock Leong
<hock.leong.kweh@xxxxxxxxx> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto@xxxxxxxxxxxxxx]
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/