Page MenuHomeFreeBSD

Work around a bug in the EFI HTTP driver
ClosedPublic

Authored by bcran on Aug 15 2019, 9:58 PM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 2 2024, 3:13 AM
Unknown Object (File)
Jan 21 2024, 4:36 PM
Unknown Object (File)
Jan 13 2024, 11:24 PM
Unknown Object (File)
Jan 5 2024, 4:38 PM
Unknown Object (File)
Dec 20 2023, 2:45 AM
Unknown Object (File)
Dec 18 2023, 5:45 PM
Unknown Object (File)
Nov 29 2023, 12:38 AM
Unknown Object (File)
Oct 18 2023, 12:57 PM

Details

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 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.

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.

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.

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.

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.

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

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.

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