From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752063AbbJETGK (ORCPT ); Mon, 5 Oct 2015 15:06:10 -0400 Received: from mail-ob0-f174.google.com ([209.85.214.174]:34453 "EHLO mail-ob0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751785AbbJETGI (ORCPT ); Mon, 5 Oct 2015 15:06:08 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Andy Lutomirski Date: Mon, 5 Oct 2015 12:05:47 -0700 Message-ID: Subject: Re: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware To: "Kweh, Hock Leong" Cc: Matt Fleming , Greg Kroah-Hartman , "Ong, Boon Leong" , LKML , "linux-efi@vger.kernel.org" , Sam Protsenko , Peter Jones , Roy Franz , Borislav Petkov , James Bottomley , Linux FS Devel , "Fleming, Matt" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 5, 2015 at 9:07 AM, Kweh, Hock Leong 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