Page MenuHomeFreeBSD

Work around a bug in the EFI HTTP driver
AcceptedPublic

Authored by bcran on Aug 15 2019, 9:58 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 OK
Unit
No Unit Test Coverage
Build Status
Buildable 25901
Build 24468: arc lint + arc unit

Event Timeline

bcran created this revision.Aug 15 2019, 9:58 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.

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.

bcran updated this revision to Diff 60859.Aug 15 2019, 11:35 PM

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.

kevans accepted this revision.Aug 16 2019, 12:10 AM

This seems to LGTM.

This revision is now accepted and ready to land.Aug 16 2019, 12:10 AM
imp added a comment.Aug 16 2019, 12:30 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.

imp accepted this revision.Aug 16 2019, 1:28 AM

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.

bcran added a comment.Mon, Aug 19, 1:35 AM

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
bcran added a comment.Mon, Aug 19, 6:43 AM

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.