Paths

Table of Contentst

Work around a bug in the EFI HTTP driver
ClosedPublic
Actions

Authored by bcran on Aug 15 2019, 9:58 PM.

Details

Reviewers

imp
scottph
tsoome
kevans
emaste

Commits

rS365505: stand/efihttp: Work around a bug in edk2 http instance reconfiguration

Summary

The EFI HTTP driver in the TianoCore EDK2 apparently doesn't expect to
deal with more than one file per instance. If asked to do so, it
corrupts memory including TLS pointers, leading to a crash.

Work around this by tearing down the HTTP instance and recreating it
before trying to access a different file.

The upstream bug report is https://bugzilla.tianocore.org/show_bug.cgi?id=1917

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25901
Build 24468: arc lint + arc unit

Event Timeline

bcran created this revision.Aug 15 2019, 9:58 PM

Harbormaster completed remote builds in B25900: Diff 60857.Aug 15 2019, 9:58 PM

bcran added reviewers: kevans, emaste.Aug 15 2019, 9:59 PM

bcran edited the summary of this revision. (Show Details)

Is this error case something that happens only sometimes? Fetching multiple files is something that has definitely worked for me before.

stand/efi/libefi/efihttp.c
578	It seems like this workaround should be moved a few lines up, before calling _efihttp_fs_open() instead of after receiving a !=0 return value from it.

In D21281#462739, @scottph wrote:

Is this error case something that happens only sometimes? Fetching multiple files is something that has definitely worked for me before.

Yes, it only happens if a file can't be found. For example if it's asked to open /boot/foo, then it'll try /boot/foo and /boot/foo/ before failing. And the crash is only seen when booting over a secure connection: HTTP works fine, HTTPS crashes.

bcran added inline comments.Aug 15 2019, 11:02 PM

stand/efi/libefi/efihttp.c
578	No, that would be wrong. In normal operation, `efihttp_fs_open` is only called once before other code ends up calling `efihttp_dev_close`. Then for the next file, `efihttp_dev_open` gets called, followed eventually by `efihttp_fs_open`. So the only time we need to explicitly close and open the dev is when we want to call `_efihttp_fs_open` twice in a row ourselves.

Fixed another place where we try and open a file without first calling
efihttp_dev_close and efihttp_dev_open.

Added a link to the TianoCore BZ ticket.

Harbormaster completed remote builds in B25901: Diff 60859.Aug 15 2019, 11:35 PM

This seems to LGTM.

This revision is now accepted and ready to land.Aug 16 2019, 12:10 AM

Seems sane enough...

Is there a performance hit to this? If so, should we only do this on error?

In D21281#462767, @imp wrote:

Is there a performance hit to this? If so, should we only do this on error?

This code only gets hit a few times per boot. The first block only gets run if a file can't be found - which is the case with some of the lua files (local.lua?). And the second block only gets run near the end of loading the kernel. So there shouldn't be any noticeable performance impact from this.
We can't call efihttp_fs_open and wait for an error, because calling it causes the crash.

OK. Not really worth optimizing then. Thanks for the feedback.

f0andrey_gmail.com added a subscriber: f0andrey_gmail.com.Aug 16 2019, 4:29 AM

In D21281#462741, @bcran wrote:

In D21281#462739, @scottph wrote:

Is this error case something that happens only sometimes? Fetching multiple files is something that has definitely worked for me before.

Yes, it only happens if a file can't be found. For example if it's asked to open /boot/foo, then it'll try /boot/foo and /boot/foo/ before failing. And the crash is only seen when booting over a secure connection: HTTP works fine, HTTPS crashes.

ok, gotcha. I kind of expected that the oh->http->Configure(NULL), oh->http->Configure(&config) dance on line 437 would be enough to clear out any fouled state. Would you mind doing one quick thing to see what's going on there? Could you print out the contents of the EFI_HTTP_CONFIG_DATA config and EFI_HTTPv4_ACCESS_POINT config_access right after oh->http->GetModeData() on line 434? We're assuming that that config data in the http object is remaining valid through connection errors and whatnot, but maybe that's not the case.

If everything looks good in the config and config_access structs then I'd agree that we're doing everything right and your change is probably the best way to work around the busted firmware.

In D21281#463005, @scottph wrote:

Would you mind doing one quick thing to see what's going on there? Could you print out the contents of the EFI_HTTP_CONFIG_DATA config and EFI_HTTPv4_ACCESS_POINT config_access right after oh->http->GetModeData() on line 434? We're assuming that that config data in the http object is remaining valid through connection errors and whatnot, but maybe that's not the case.

Huh, I'm seeing some really odd stuff in there right from the very first time we call it, way before the crash (and when connecting using both HTTP and HTTPS):

config.
HttpVersion=1
TimeOutMillisec=0
LocalAddressIsIPv6=0
AccessPoint.IPv4Node.UseDefaultAddress=1
AccessPoint.IPv4Node.LocalAddress=168.0.87.255
AccessPoint.IPv4Node.LocalSubnet=255.255.0.20
AccessPoint.IPv4Node.LocalPort=0

config_access.
AccessPoint.IPv4Node.UseDefaultAddress=1
AccessPoint.IPv4Node.LocalAddress=168.0.87.255
AccessPoint.IPv4Node.LocalSubnet=255.255.0.20
AccessPoint.IPv4Node.LocalPort=0

In D21281#463691, @bcran wrote:

Huh, I'm seeing some really odd stuff in there right from the very first time we call it, way before the crash (and when connecting using both HTTP and HTTPS):

It's looking more like we're corrupting stuff. When I run the following code, I'm seeing the contents of config_access change:

	printf("config_access:\n");
	printf("LocalAddress: %d.%d.%d.%d\n",
			config_access.LocalAddress.Addr[0],
			config_access.LocalAddress.Addr[1],
			config_access.LocalAddress.Addr[2],
			config_access.LocalAddress.Addr[3]);
	printf("SubnetMask: %d.%d.%d.%d\n",
			config_access.LocalSubnet.Addr[0],
			config_access.LocalSubnet.Addr[1],
			config_access.LocalSubnet.Addr[2],
			config_access.LocalSubnet.Addr[3]);

	err = setup_ipv4_config2(handle, mac, ipv4, dns);
	if (err)
		return (err);

	printf("config_access2:\n");
	printf("LocalAddress2: %d.%d.%d.%d\n",
			config_access.LocalAddress.Addr[0],
			config_access.LocalAddress.Addr[1],
			config_access.LocalAddress.Addr[2],
			config_access.LocalAddress.Addr[3]);
	printf("SubnetMask2: %d.%d.%d.%d\n",
			config_access.LocalSubnet.Addr[0],
			config_access.LocalSubnet.Addr[1],
			config_access.LocalSubnet.Addr[2],
			config_access.LocalSubnet.Addr[3]);

Inside setup_ipv4_config2 all I'm doing is:

		printf("IPv4 LocalIpAddress: %d.%d\n",
				ipv4->LocalIpAddress.Addr[0],
				ipv4->LocalIpAddress.Addr[1]);

config_access:                                                                                                                                              
LocalAddress: 0.0.0.0                                                                                                                                       
SubnetMask: 0.0.0.0                                                                                                                                         
IPv4 LocalIpAddress: 192.168                                                                                                                                
config_access2:                                                                                                                                              
LocalAddress2: 230.230.127.0                                                                                                                                 
SubnetMask2: 0.0.0.240

About the EFI_HTTP_Config_Data:

"It is the responsibility of the caller to allocate the memory for HttpConfigData and HttpConfigData- >AccessPoint.IPv6Node/IPv4Node. In fact, it is recommended to allocate sufficient memory to record IPv6Node since it is big enough for all possibilities."

Therefore even is we inly use v4, allocate size based on v6.

I've tracked this down to a particular use-after-free type bug in edk2: https://www.mail-archive.com/devel@edk2.groups.io/msg25730.html

It probably would make sense to re-work the lifecycle of the http instance after this change, but I'd say going ahead with this to bandaid https makes sense.

tsoome accepted this revision.Sep 8 2020, 5:13 AM

Closed by commit rS365505: stand/efihttp: Work around a bug in edk2 http instance reconfiguration (authored by scottph). · Explain WhySep 9 2020, 4:36 PM

This revision was automatically updated to reflect the committed changes.

scottph added a commit: rS365505: stand/efihttp: Work around a bug in edk2 http instance reconfiguration.

Thanks for committing this, @scottph! I've been busy with a new job so haven't had time to work on FreeBSD recently.