| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 5 Oct 2015 12:05:47 -0700
From: Andy Lutomirski <luto@...capital.net>
To: "Kweh, Hock Leong" <hock.leong.kweh@...el.com>
Cc: Matt Fleming <matt@...sole-pimps.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Ong, Boon Leong" <boon.leong.ong@...el.com>,
LKML <linux-kernel@...r.kernel.org>,
"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
Sam Protsenko <semen.protsenko@...aro.org>,
Peter Jones <pjones@...hat.com>,
Roy Franz <roy.franz@...aro.org>,
Borislav Petkov <bp@...en8.de>,
James Bottomley <James.Bottomley@...senpartnership.com>,
Linux FS Devel <linux-fsdevel@...r.kernel.org>,
"Fleming, Matt" <matt.fleming@...el.com>
Subject: Re: [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@...el.com> wrote:
>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto@...capital.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists