Bug 1196114 - buffer overflow in fallback.c, add_boot_option() and find_boot_option()
Summary: buffer overflow in fallback.c, add_boot_option() and find_boot_option()
Keywords:
Status: CLOSED EOL
Alias: None
Product: Fedora
Classification: Fedora
Component: shim
Version: 26
Hardware: All
OS: Linux
unspecified
unspecified
Target Milestone: ---
Assignee: Peter Jones
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1362257 (view as bug list)
Depends On:
Blocks: 1362257
TreeView+ depends on / blocked
 
Reported: 2015-02-25 10:43 UTC by Richard W.M. Jones
Modified: 2018-05-29 12:02 UTC (History)
10 users (show)

Fixed In Version:
Clone Of:
: 1362257 1373851 (view as bug list)
Environment:
Last Closed: 2018-05-29 12:02:21 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)
Full boot log (59.79 KB, text/plain)
2015-02-25 10:43 UTC, Richard W.M. Jones
no flags Details
Boot log with upstream shim + DEBUG_FALLBACK (53.64 KB, text/plain)
2015-02-25 13:32 UTC, Richard W.M. Jones
no flags Details
allocate.patch (1.05 KB, patch)
2015-02-25 17:16 UTC, Richard W.M. Jones
no flags Details | Diff
Fix length of allocated buffer for boot option comparison. (1.80 KB, patch)
2015-03-05 20:13 UTC, Laszlo Ersek
no flags Details | Diff

Description Richard W.M. Jones 2015-02-25 10:43:11 UTC
Created attachment 995091 [details]
Full boot log

Description of problem:

(This is actually on Fedora, but there is no AAVMF component
for Fedora)

$ virt-builder fedora-21 --root-password password:123456 --install shim
$ cp /usr/share/AAVMF/AAVMF_VARS.fd /tmp/vars.fd 
$ qemu-system-aarch64 -nodefconfig -nodefaults -display none -M virt -cpu host -machine accel=kvm -m 2048 -no-reboot -drive if=pflash,format=raw,file=/usr/share/AAVMF/AAVMF_CODE.fd,readonly -drive if=pflash,format=raw,file=/tmp/vars.fd -device virtio-scsi-device,id=scsi -drive file=fedora-21.img,format=raw,if=none,id=hd0 -device scsi-hd,drive=hd0 -serial stdio |& tee /tmp/log

Because of bug 1190191 this won't actually boot.  However it
drops you into the UEFI shell.

Running \EFI\BOOT\fallback.efi results in:

FSOpen: Open '\efi\boot\fallback.efi' Success
FSOpen: Open '\efi\boot\fallback.efi' Success
FSOpen: Open '\efi\boot\fallback.efi' Success
FSOpen: Open '\efi\boot\fallback.efi' Success
InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B BBB7F040
Loading driver at 0x000BF4F8000 EntryPoint=0x000BF4F8148
Loading driver at 0x000BF4F8000 EntryPoint=0x000BF4F8148 
InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF BBB14018
InstallProtocolInterface: 752F3136-4E16-4FDC-A22A-E5F46812F4CA BF7169C0
System BootOrder not found.  Initializing defaults.
FSOpen: Open 'EFI' Success
FSOpen: Open 'fedora' Success
FSOpen: Open 'BOOT.CSV' Success
FSOpen: Open '\EFI\fedora\BOOT.CSV' Success
ASSERT /builddir/build/BUILD/aavmf-77d5dac/MdeModulePkg/Core/Dxe/Mem/Pool.c(488): Tail->Signature == ((('p') | ('t' << 8)) | ((('a') | ('l' << 8)) << 16))

Version-Release number of selected component (if applicable):

AAVMF-20141113-4.git77d5dac.aa7a.aarch64

How reproducible:

100%

Steps to Reproduce:
1. See above.

Comment 1 Laszlo Ersek 2015-02-25 11:06:12 UTC
Please upload the compressed disk image somewhere. Also please identify the exact shim version number (Koji URL, dist-git commit etc. would be preferable). I need to reproduce this and instrument and rebuild fallback.efi to see where it blows up.

The ASSERT in comment #0 bears witness to the corruption of the memory allocator's internal structures. That can be caused by a double free or another memory management issue. The error message doesn't clearly finger either component, so we need to debug into fallback.efi, see if (and where) it triggers (but doesn't necessarily cause) the error.

Thanks
Laszlo

Comment 4 Richard W.M. Jones 2015-02-25 13:32:46 UTC
Created attachment 995211 [details]
Boot log with upstream shim + DEBUG_FALLBACK

I compiled the upstream shim (361716dd4a612)
+ Peter Jones's patch https://bugzilla.redhat.com/show_bug.cgi?id=1190191#c12
+ enabling DEBUG_FALLBACK
+ a few trivial code fixes to make it all compile

The resulting log is attached.  It fails in exactly the same
way, but with a bit of extra debugging.

Comment 5 Richard W.M. Jones 2015-02-25 15:21:13 UTC
I can't quite see the error yet, but it is triggered in the
shim fallback.c function find_boot_option, at:

        FreePool(candidate);
        FreePool(data);     <<<<<<<<
        return EFI_NOT_FOUND;
}

Comment 6 Laszlo Ersek 2015-02-25 16:22:14 UTC
Thanks!

Looking again at the assert, it seems to be complaining about an overwritten
magic footer in the dynamically allocated block, at release time. So let's
see what could overflow here:

        unsigned int size = sizeof(UINT32) + sizeof (UINT16) +
                StrLen(label)*2 + 2 + DevicePathSize(dp) +
                StrLen(arguments) * 2;

        CHAR8 *data = AllocateZeroPool(size);
        if (!data)
                return EFI_OUT_OF_RESOURCES;
        CHAR8 *cursor = data;
        *(UINT32 *)cursor = LOAD_OPTION_ACTIVE;
        cursor += sizeof (UINT32);
        *(UINT16 *)cursor = DevicePathSize(dp);
        cursor += sizeof (UINT16);
        StrCpy((CHAR16 *)cursor, label);
        cursor += StrLen(label)*2 + 2;
        CopyMem(cursor, dp, DevicePathSize(dp));
        cursor += DevicePathSize(dp);
        StrCpy((CHAR16 *)cursor, arguments);

I think the "size" calculation does not include the final L'\0', for the
"arguments" StrCpy(). That is, the last two bytes (the L'\0' in UCS-2)
written by the final StrCpy() overflows "data".

Can you please try the following patch:

> diff --git a/fallback.c b/fallback.c
> index d10fb62..da54af4 100644
> --- a/fallback.c
> +++ b/fallback.c
> @@ -232,7 +232,7 @@ find_boot_option(EFI_DEVICE_PATH *dp, EFI_DEVICE_PATH *fulldp,
>  {
>  	unsigned int size = sizeof(UINT32) + sizeof (UINT16) +
>  		StrLen(label)*2 + 2 + DevicePathSize(dp) +
> -		StrLen(arguments) * 2;
> +		StrLen(arguments) * 2 + 2;
>  
>  	CHAR8 *data = AllocateZeroPool(size);
>  	if (!data)

Comment 7 Laszlo Ersek 2015-02-25 16:27:42 UTC
Bug introduced by:

commit 4aac8a1179e160397d7ef8f1e3232cfb4f3373d6
Author: Gary Ching-Pang Lin <glin>
Date:   Thu Mar 6 10:57:02 2014 +0800

    [fallback] Fix the data size for boot option comparison
    
    Signed-off-by: Gary Ching-Pang Lin <glin>

Ugh... no?

Comment 8 Laszlo Ersek 2015-02-25 16:39:13 UTC
Ah, I do see the point of 4aac8a1179e160397d7ef8f1e3232cfb4f3373d6. Too bad it didn't only change the size used for *comparison*, it also decreased it for *allocation*. So forget the patch in comment 6, try this instead please:

> diff --git a/fallback.c b/fallback.c
> index d10fb62..b7e4133 100644
> --- a/fallback.c
> +++ b/fallback.c
> @@ -234,7 +234,7 @@ find_boot_option(EFI_DEVICE_PATH *dp, EFI_DEVICE_PATH *fulldp,
>  		StrLen(label)*2 + 2 + DevicePathSize(dp) +
>  		StrLen(arguments) * 2;
>  
> -	CHAR8 *data = AllocateZeroPool(size);
> +	CHAR8 *data = AllocateZeroPool(size + 2);
>  	if (!data)
>  		return EFI_OUT_OF_RESOURCES;
>  	CHAR8 *cursor = data;

Comment 9 Richard W.M. Jones 2015-02-25 17:16:12 UTC
Created attachment 995274 [details]
allocate.patch

Do we also need to change this?

-       CHAR8 *candidate = AllocateZeroPool(size);                          
+       CHAR8 *candidate = AllocateZeroPool(size + 2);                          

---

Anyway, it's not quite fixed.  That fixes the first bug, but it then
fails later on in the add_boot_option function:

Creating boot entry "Boot0004" with label "Fedora" for file "\EFI\fedora\shim.efi"
ASSERT /builddir/build/BUILD/aavmf-77d5dac/MdeModulePkg/Core/Dxe/Mem/Pool.c(488): Tail->Signature == ((('p') | ('t' << 8)) | ((('a') | ('l' << 8)) << 16))

(note that previously it didn't print "Creating boot entry ...")

There is another dubious size calculation in add_boot_option, so
I adjusted the allocation there too.

The full patch I'm using is attached.

Comment 10 Richard W.M. Jones 2015-02-25 17:19:18 UTC
So with the patch in comment 9, I'm not actually sure if it's working
or not.

It now goes into a blue menu that says <<
  Shim UEFI key management
  Press any key to perform MOK management
  Booting in [10] seconds
>>

Then I get into a bunch of menus that I have to navigate by
hand (please don't do this -- libguestfs doesn't have a human
who can navigate menus).

Even with navigating the menus, I'm still unable to get the
guest to boot.

Comment 12 Laszlo Ersek 2015-03-05 20:13:08 UTC
Created attachment 998539 [details]
Fix length of allocated buffer for boot option comparison.

Comment 13 Jan Kurik 2015-07-15 14:29:49 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 23 development cycle.
Changing version to '23'.

(As we did not run this process for some time, it could affect also pre-Fedora 23 development
cycle bugs. We are very sorry. It will help us with cleanup during Fedora 23 End Of Life. Thank you.)

More information and reason for this action is here:
https://fedoraproject.org/wiki/BugZappers/HouseKeeping/Fedora23

Comment 14 Robert Elliott 2015-09-03 15:16:24 UTC
Fedora 22 triggers this bug on HP ProLiant Gen9 systems (UEFI-based) that have no boot path, such as after BIOS settings have been reset to their default values.

Comment 15 Pavel Butsykin 2016-10-26 09:18:15 UTC
I see a similar problem in Fedora24 (Fedora-Server-dvd-x86_64-24-1.2.iso):

[root@localhost ~]# cat /etc/*-release
Fedora release 24 (Twenty Four)
NAME=Fedora
VERSION="24 (Server Edition)"
ID=fedora
VERSION_ID=24
PRETTY_NAME="Fedora 24 (Server Edition)"
ANSI_COLOR="0;34"
CPE_NAME="cpe:/o:fedoraproject:fedora:24"
HOME_URL="https://fedoraproject.org/"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Fedora"
REDHAT_BUGZILLA_PRODUCT_VERSION=24
REDHAT_SUPPORT_PRODUCT="Fedora"
REDHAT_SUPPORT_PRODUCT_VERSION=24
PRIVACY_POLICY_URL=https://fedoraproject.org/wiki/Legal:PrivacyPolicy
VARIANT="Server Edition"
VARIANT_ID=server
Fedora release 24 (Twenty Four)

log:
...
FSOpen: Open '\EFI\BOOT\BOOTX64.EFI' Success
MnpReceivePacket: Size error, HL:TL = 1073234448:1536.
FSOpen: Open '\EFI\BOOT\BOOTX64.EFI' Success
MnpReceivePacket: Size error, HL:TL = 1073234448:1536.
InstallProtocolInterface: 5B1B31A1-9562-11D2-8E3F-00A0C969723B 3F087AC0
Loading driver at 0x0003E333000 EntryPoint=0x0003E34F000 
InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 3F084B98
InstallProtocolInterface: 605DAB50-E046-4300-ABB6-3DD810DD8B23 3E41DC90
FSOpen: Open '\EFI\BOOT\fallback.efi' Success
FSOpen: Open '\EFI\BOOT\fallback.efi' Success
FSOpen: Open 'EFI' Success
FSOpen: Open 'fedora' Success
FSOpen: Open 'BOOT.CSV' Success
FSOpen: Open '\EFI\fedora\BOOT.CSV' Success
ASSERT /builddir/build/BUILD/ovmf-c9e5618/MdeModulePkg/Core/Dxe/Mem/Pool.c(548): Tail->Signature == ((('p') | ('t' << 8)) | ((('a') | ('l' << 8)) << 16))

Comment 16 Laszlo Ersek 2016-10-27 11:52:18 UTC
Okay, I just realized the following: Peter silently merged my patch from comment 12 into upstream shim; it is:

commit 6b2510522f92026dc17e1c5508bbfab935741012
Author: Laszlo Ersek <lersek>
Date:   Wed Feb 25 18:45:41 2015 +0000

    Fix length of allocated buffer for boot option comparison.

That commit is part of release v0.9 -- see also Dan's comment in bug 1362257 comment 1 -- so all we seem to need here is an RPM upgrade in Fedora, to upstream v0.9 (or later).

Comment 17 Laszlo Ersek 2016-10-27 11:54:21 UTC
*** Bug 1362257 has been marked as a duplicate of this bug. ***

Comment 18 Laszlo Ersek 2016-10-27 15:48:07 UTC
From <https://bugzilla.tianocore.org/show_bug.cgi?id=189#c4>:

Jan Sedlák 2016-10-27 16:20:31 CEST
> Thanks, I talked to pjones and he told me that they're planning to upgrade
> Fedora to shim 0.10 somewhere around F26 alpha.

Comment 19 Gerd Hoffmann 2017-06-29 16:28:56 UTC
(In reply to Laszlo Ersek from comment #18)
> From <https://bugzilla.tianocore.org/show_bug.cgi?id=189#c4>:
> 
> Jan Sedlák 2016-10-27 16:20:31 CEST
> > Thanks, I talked to pjones and he told me that they're planning to upgrade
> > Fedora to shim 0.10 somewhere around F26 alpha.

Ping.  F26 still is at shim-0.8-10.x86_64 ...

Comment 20 Fedora End Of Life 2017-07-25 18:50:41 UTC
This message is a reminder that Fedora 24 is nearing its end of life.
Approximately 2 (two) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 24. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '24'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not
able to fix it before Fedora 24 is end of life. If you would still like
to see this bug fixed and are able to reproduce it against a later version
of Fedora, you are encouraged  change the 'version' to a later Fedora
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

Comment 21 Gerd Hoffmann 2017-07-26 05:32:00 UTC
> Ping.  F26 still is at shim-0.8-10.x86_64 ...

Moving to F26.

Comment 22 Fedora End Of Life 2018-05-03 08:52:35 UTC
This message is a reminder that Fedora 26 is nearing its end of life.
Approximately 4 (four) weeks from now Fedora will stop maintaining
and issuing updates for Fedora 26. It is Fedora's policy to close all
bug reports from releases that are no longer maintained. At that time
this bug will be closed as EOL if it remains open with a Fedora  'version'
of '26'.

Package Maintainer: If you wish for this bug to remain open because you
plan to fix it in a currently maintained version, simply change the 'version'
to a later Fedora version.

Thank you for reporting this issue and we are sorry that we were not
able to fix it before Fedora 26 is end of life. If you would still like
to see this bug fixed and are able to reproduce it against a later version
of Fedora, you are encouraged  change the 'version' to a later Fedora
version prior this bug is closed as described in the policy above.

Although we aim to fix as many bugs as possible during every release's
lifetime, sometimes those efforts are overtaken by events. Often a
more recent Fedora release includes newer upstream software that fixes
bugs or makes them obsolete.

Comment 23 Fedora End Of Life 2018-05-29 12:02:21 UTC
Fedora 26 changed to end-of-life (EOL) status on 2018-05-29. Fedora 26
is no longer maintained, which means that it will not receive any
further security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.


Note You need to log in before you can comment on or make changes to this bug.