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

* RE: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware
@ 2015-10-05 16:07 Kweh, Hock Leong
  2015-10-05 19:05 ` Andy Lutomirski
  0 siblings, 1 reply; 6+ messages in thread
From: Kweh, Hock Leong @ 2015-10-05 16:07 UTC (permalink / raw)
  To: Andy Lutomirski
  Cc: Matt Fleming, Greg Kroah-Hartman, Ong, Boon Leong, LKML,
	linux-efi, Sam Protsenko, Peter Jones, Roy Franz,
	Borislav Petkov, James Bottomley, Linux FS Devel, Fleming, Matt

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4589 bytes --]

> -----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.

> 
> > +               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. So,
user app is expected to upload the image binary sequentially.

Also captured this into efi_capsule_write() comment in v7 patchset: 
https://lkml.org/lkml/2015/10/5/232

> 
> > +       }
> > +
> > +       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.

> 
> > +       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.

Thanks & Regards,
Wilson

ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éÝ¶\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dÊ‡Ú™ë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 6+ messages in thread