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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@xxxxxxxxxxxxxx]
> Sent: Tuesday, October 06, 2015 3:06 AM
>
> >> 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).

Yes, I could add a condition checking for this:
if (write_byte < sizeof(efi_capsule_header_t) { ... }
to ensure the 1st block count does not less than the capsule header size.
If not, will return error.

Do you have any idea that in what kind of situation user app will pass in less than
28 bytes each time?

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

Answered as above.

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

Yes, the allocation is at:
cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
                                                                 GFP_KERNEL);
before line:
cap_info.header_obtained = 1;

> 
> >> > +               }
> >> > +               /* 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?
> 

I think it is still okay as the data is still within a page and this could cater the image
binary that padding to page size. Whatever next write that more than the current
page, will return error -EIO.

If you think that should flag an error, I could simply add the condition checking
to it.


Thanks & Regards,
Wilson

N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ0¶ìå