Bug 1196114 - buffer overflow in fallback.c, add_boot_option() and find_boot_option()
buffer overflow in fallback.c, add_boot_option() and find_boot_option()
Status: NEW
Product: Fedora
Classification: Fedora
Component: shim (Show other bugs)
24
All Linux
unspecified Severity unspecified
: ---
: ---
Assigned To: Peter Jones
Fedora Extras Quality Assurance
:
: 1362257 (view as bug list)
Depends On:
Blocks: 1362257
  Show dependency treegraph
 
Reported: 2015-02-25 05:43 EST by Richard W.M. Jones
Modified: 2017-06-29 12:28 EDT (History)
10 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 1362257 1373851 (view as bug list)
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


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

  None (edit)
Description Richard W.M. Jones 2015-02-25 05:43:11 EST
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 06:06:12 EST
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 08:32:46 EST
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 10:21:13 EST
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 11:22:14 EST
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 11:27:42 EST
Bug introduced by:

commit 4aac8a1179e160397d7ef8f1e3232cfb4f3373d6
Author: Gary Ching-Pang Lin <glin@suse.com>
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@suse.com>

Ugh... no?
Comment 8 Laszlo Ersek 2015-02-25 11:39:13 EST
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 12:16:12 EST
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 12:19:18 EST
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 15:13:08 EST
Created attachment 998539 [details]
Fix length of allocated buffer for boot option comparison.
Comment 13 Jan Kurik 2015-07-15 10:29:49 EDT
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 11:16:24 EDT
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 05:18:15 EDT
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 07:52:18 EDT
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@redhat.com>
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 07:54:21 EDT
*** Bug 1362257 has been marked as a duplicate of this bug. ***
Comment 18 Laszlo Ersek 2016-10-27 11:48:07 EDT
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 12:28:56 EDT
(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 ...

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