Uninstalling earlier shim protocol does not actually do anything, fails silently

[julian-klode]

While analyzing https://bugs.launchpad.net/bugs/1865515 I found out that the earlier shim protocol uninstalling actually does nothing.

In src/shim.c inside install_shim_protocols() we attempt to uninstall the old shim by calling uninstall_shim_protocols(). This does nothing, and hence loading two shims and then a kernel fails.

What happens is that when we enter uninstall_shim_protocols() we have shim_lock_handle == NULL, which is an invalid argument (and hence UninstallProtocolInterface() fails with invalid argument error). It's NULL because we have not yet installed our new shim protocol, or made any attempt to get the old handle.

Furthermore, the protocol interface we try to remove in there is shim_lock_interface, but that is our new interface - the old one we located was only stored in a local variable in shim_lock.

How can we fix this?

One solution I found involves asking the old shim to uninstall itself, by extending its protocol (or realistically, adding a new one, for ABI reasons I suppose). The old shim knows its handle and interface, and hence uninstall_shim_protocols there just works.

Another attempt could be made by locating the old shim's handle with LibLocateHandle() or LocateHandle() or LocateHandleBuffer() similar to how we locate the protocol using LibLocateProtocol(), and then uninstall that.

We should also add error checking to the uninstall code, so we actually see errors there as well.

[julian-klode]

This basically means commit c8ca1c569664913e580340105bceb75b5aecad57 by @cyphermox never did anything, but this is confusing :/

[vathpela]

I have an idea for how to make this a lot better, but it requires a couple of other features first: Add two protocols that shim provides and installs:

SHIM_VOLATILE_KEYS_PROTOCOL:

• this is a wrapper for GetVariable/SetVariable/GetNextVariableName which defers all runtime + volatile variables with VendorGuid == SHIM_LOCK_GUID into an EFI configuration table instead
• the purpose of this is to provide an API extend db/dbx/kek in ram on a volatile basis that any code that chooses can consume
• right now that means the API is something like:
  • vk->SetVariable() - basically the same semantics as systab->SetVariable()
  • VendorGuid other than SHIM_LOCK_GUID => pass through to systab->SetVariable()
  • Attributes other than BS|RT or BS|RT|APPEND => pass through to systab->SetVariable()
  • if {VendorGuid,VariableName} exists in system variables => pass through to systab->SetVariable()
  • vk->GetVariable() and vk->GetNextVariableName() - just so we can let things use 1 variable API and make it check our variables before the system variable store
  • install only if it's not found; if we need new versions we rev the protocol guid like UEFI does in general
  • the config variable has the format implemented by the following patches
  • https://github.com/rhboot/shim/commit/fecc2dfb8e408526221091923d9345796b8e294e https://github.com/rhboot/shim/commit/65be350308783a8ef537246c8ad0545b4e6ad069 https://github.com/rhboot/shim/commit/63f7943dbe0583dd0bf89ee8fe8230d4b7373b91
  • we wrote this because of the list of RHEL 7 kernel MokList hashes, which surprisingly got us EFI_INVALID_PARAMETER trying to set MokListRT, so we need it anyway
  • lennysz@rh has written a kernel patch to consume this for MokList/MokListX/etc and propagate it to userland for mokutil to consume, currently based on that first commit above; we'll work to upstream it
  • when shim starts, (maybe) installs a version of this, and then it uses the installed copy (wherever it's from) to update MokListRT/MokListXRT/etc with any entries that are missing from any of the lists (or in the future, when it loads keys from anywhere) which it knows about internally
  • the original idea for Volatile Keys is old, but at the time there were... political problems with it. They have since somewhat gone away, and some of it has changed because of the other bits of MokList/MokListX and doing this in shim instead of in UEFI proper, but the original proposal is here: https://drive.google.com/file/d/0B9HBBTb3mZKdbnVqV1Y3c1lpdGs/view?usp=sharing
  • I really, really wish we'd gotten this in.

SHIM_IMAGE_PROTOCOL:

• provides its own functions for:
  • LoadImage()
  • StartImage()
  • Exit()
  • UnloadImage()
  • ExitBootServices()
• LoadImage()/StartImage() code here fulfills the participation test that's currently implemented in replacements.c
  • this gets us real subclassing of LoadImage()/StartImage(), which we want very badly anyway but can't do otherwise because of (horrifying) ABI-related reasons.
  • make LoadImage() install a handle on the loaded image's LoadedImage ... more on that below
  • if that's missing, use the original ones from the systab
  • this gets us real subclassing of LoadImage()/StartImage(), which we want very badly anyway but can't do otherwise because of (horrifying) ABI-related reasons
  • which means we can also load binaries that we've audited to make sure e.g. they don't have a PE loader, a module loader, or a programming language that allows arbitrary memory access, and aren't SB-aware, and have them reasonably trusted.

the handle on the LoadedImage object this is basically just a pointer to a struct that's opaque to everyone but shim, but it gives us:

• a pointer to the SHIM_IMAGE_PROTOCOL instance from the shim loader that actually loaded this binary
• a pointer to the SHIM_VOLATILE_KEYS_PROTOCOL from the correct shim
• a pointer to the SHIM_LOCK_PROTOCOL from the correct shim if the handle isn't installed, it was loaded either by a) some chained shim-type thing from farther down the stack which doesn't participate in these calls or b) the original systab->LoadImage()
• in either case, we use the systab-> versions of all the IMAGE calls, or fall back to the current replacements strategy that tells us which SHIM_IMAGE_PROTOCOL instance it should get for purposes of StartImage() etc