efirt: enter runtime environment to deref efi_cfgtbl

[kevans]

Summary

This fixes an insta-panic when EFIIOC_GET_TABLE is used.

@jhb points out that we probably want an alternative to exporting this pointer into KVA back to userland, but I defer on that one.

[imp]

This looks like it would fix things, but I wonder why this is even here... If we keep it, this change prevents a crash... Though I can't for the life of me think about the purpose this serves... It went in with a bunch of efi env var support.

[jhb]

Yeah, this is a good fix to get in first in case we resurrect the ioctl in some other form in the future.

[jrtc27]

I believe the ioctl is the only consumer of this function though, and all others use the contrib efi version of the function?

[kib]

efi_enter() is too heavy-weight to read a pointer. Why not apply more efi_phys_to_kva() ?

On the other hand, efi_phys_to_kva() itself can only work on machines with DMAP, whatever it implementation is. If you do efi_enter(), you do not need efi_phys_to_kva().

[kevans]

In D27669#618624, @kib wrote:

efi_enter() is too heavy-weight to read a pointer. Why not apply more efi_phys_to_kva() ?

On the other hand, efi_phys_to_kva() itself can only work on machines with DMAP, whatever it implementation is. If you do efi_enter(), you do not need efi_phys_to_kva().

My original approach did just efi_phys_to_kva(efi_cfgtbl), but then I read the comment (which I apparently wrote?) near efi_cfgtbl's definition and I couldn't remember what I might be overlooking.

[jrtc27]

Or just remove it and the ioctl given it's never used in the base system (and if it's always been broken can never have been used by anyone outside the base system either)?

[kib]

In D27669#618625, @kevans wrote:

In D27669#618624, @kib wrote:

efi_enter() is too heavy-weight to read a pointer. Why not apply more efi_phys_to_kva() ?

On the other hand, efi_phys_to_kva() itself can only work on machines with DMAP, whatever it implementation is. If you do efi_enter(), you do not need efi_phys_to_kva().

My original approach did just efi_phys_to_kva(efi_cfgtbl), but then I read the comment (which I apparently wrote?) near efi_cfgtbl's definition and I couldn't remember what I might be overlooking.

I can think only of one specific situation where DMAP would fail. Namely, if EFI_CONFIGURATION_TABLE element overflows into the next page.

Otherwise, the comment you mention just means that addresses are from efirt mappings, but we explicitly use 1:1 mappings both on amd64 and on arm64.

[kevans]

In D27669#618626, @jrtc27 wrote:

Or just remove it and the ioctl given it's never used in the base system (and if it's always been broken can never have been used by anyone outside the base system either)?

It seems a little more difficult to assert that efi_get_table() couldn't have been used outside of the base system; this KPI has existed for four years and a caller could have easily mapped or had mapped the appropriate physical range to use it, no?

[jrtc27]

In D27669#618631, @kevans wrote:

In D27669#618626, @jrtc27 wrote:

Or just remove it and the ioctl given it's never used in the base system (and if it's always been broken can never have been used by anyone outside the base system either)?

It seems a little more difficult to assert that efi_get_table() couldn't have been used outside of the base system; this KPI has existed for four years and a caller could have easily mapped or had mapped the appropriate physical range to use it, no?

If that's a concern then you can't add efi_enter/efi_leave as it's not reentrant (or maybe the enter side works but the leave doesn't know it's nested) and will break any such code using it at run time?

[kevans]

In D27669#618632, @jrtc27 wrote:

In D27669#618631, @kevans wrote:

In D27669#618626, @jrtc27 wrote:

Or just remove it and the ioctl given it's never used in the base system (and if it's always been broken can never have been used by anyone outside the base system either)?

It seems a little more difficult to assert that efi_get_table() couldn't have been used outside of the base system; this KPI has existed for four years and a caller could have easily mapped or had mapped the appropriate physical range to use it, no?

If that's a concern then you can't add efi_enter/efi_leave as it's not reentrant (or maybe the enter side works but the leave doesn't know it's nested) and will break any such code using it at run time?

efi_enter/efi_leave are private to efirt.c, there's no route for this to cause a recursion on the efi lock. The main thing I'm wondering is if it's feasible that an external caller could have mapped a range covering these pages such that dereferencing was not an issue, whether said mapping was intentional or not.

[kib]

In D27669#620267, @kevans wrote:

The main thing I'm wondering is if it's feasible that an external caller could have mapped a range covering these pages such that dereferencing was not an issue, whether said mapping was intentional or not.

I do not quite understand what are you worries there. efi_enter() switches address space so that EFI RT pointers become valid. Whatever was mapped at VA of RT is invalid until efi_leave().

Point I tried to make in earlier responses, possibly not clean enough, is that it is not needed to use efi_phys_to_kva() once efi_enter() is active. In theory, something from KVA might be even unmapped while efi_enter() is active, but in practice our current assumption is that everything in RT is in UVA. But I prefer to avoid unneeded efi_phys_to_kva() to have this code more future-proof.

[kevans]

In D27669#620309, @kib wrote:

In D27669#620267, @kevans wrote:

The main thing I'm wondering is if it's feasible that an external caller could have mapped a range covering these pages such that dereferencing was not an issue, whether said mapping was intentional or not.

I do not quite understand what are you worries there. efi_enter() switches address space so that EFI RT pointers become valid. Whatever was mapped at VA of RT is invalid until efi_leave().

Sorry, the specific question I'm trying to answer is if out-of-tree kernel code could have successfully called efi_get_table() in the past four years (accidentally or intentionally) of its life despite the fact that efi_get_table() did not itself do this.

Point I tried to make in earlier responses, possibly not clean enough, is that it is not needed to use efi_phys_to_kva() once efi_enter() is active. In theory, something from KVA might be even unmapped while efi_enter() is active, but in practice our current assumption is that everything in RT is in UVA. But I prefer to avoid unneeded efi_phys_to_kva() to have this code more future-proof.

Right, so my interpretation between your past comments and this one is that we should either efi_enter() and drop the redundant efi_phys_to_kva() inside or just grab the cfg table pointer with efi_phys_to_kva() and not bother with efi_enter(). What I gathered was that your initial instinct was the latter since efi_enter() is heavyweight, but that the former would probably be better here because:

I can think only of one specific situation where DMAP would fail. Namely, if EFI_CONFIGURATION_TABLE element overflows into the next page.

[kib]

In D27669#620337, @kevans wrote:

In D27669#620309, @kib wrote:

In D27669#620267, @kevans wrote:

The main thing I'm wondering is if it's feasible that an external caller could have mapped a range covering these pages such that dereferencing was not an issue, whether said mapping was intentional or not.

I do not quite understand what are you worries there. efi_enter() switches address space so that EFI RT pointers become valid. Whatever was mapped at VA of RT is invalid until efi_leave().

Sorry, the specific question I'm trying to answer is if out-of-tree kernel code could have successfully called efi_get_table() in the past four years (accidentally or intentionally) of its life despite the fact that efi_get_table() did not itself do this.

Why should we care about this ?
That said, I do not think there exists out of tree code using efi*.

Point I tried to make in earlier responses, possibly not clean enough, is that it is not needed to use efi_phys_to_kva() once efi_enter() is active. In theory, something from KVA might be even unmapped while efi_enter() is active, but in practice our current assumption is that everything in RT is in UVA. But I prefer to avoid unneeded efi_phys_to_kva() to have this code more future-proof.

Right, so my interpretation between your past comments and this one is that we should either efi_enter() and drop the redundant efi_phys_to_kva() inside or just grab the cfg table pointer with efi_phys_to_kva() and not bother with efi_enter(). What I gathered was that your initial instinct was the latter since efi_enter() is heavyweight, but that the former would probably be better here because:

I can think only of one specific situation where DMAP would fail. Namely, if EFI_CONFIGURATION_TABLE element overflows into the next page.

I will be happy if you just remove efi_phys_to_kva() in the scope of this patch.

[kevans]

In D27669#620346, @kib wrote:

In D27669#620337, @kevans wrote:

In D27669#620309, @kib wrote:

I do not quite understand what are you worries there. efi_enter() switches address space so that EFI RT pointers become valid. Whatever was mapped at VA of RT is invalid until efi_leave().

Sorry, the specific question I'm trying to answer is if out-of-tree kernel code could have successfully called efi_get_table() in the past four years (accidentally or intentionally) of its life despite the fact that efi_get_table() did not itself do this.

Why should we care about this ?
That said, I do not think there exists out of tree code using efi*.

The only reason I cared was to determine if it's even worth fixing, rather than just removing it and MFC'ing that if it's really unusable and suspected to not be used at all since the ioctl interface needs to be reimagined anyways; but I think I'm just getting hung up on details here, and...

Point I tried to make in earlier responses, possibly not clean enough, is that it is not needed to use efi_phys_to_kva() once efi_enter() is active. In theory, something from KVA might be even unmapped while efi_enter() is active, but in practice our current assumption is that everything in RT is in UVA. But I prefer to avoid unneeded efi_phys_to_kva() to have this code more future-proof.

Right, so my interpretation between your past comments and this one is that we should either efi_enter() and drop the redundant efi_phys_to_kva() inside or just grab the cfg table pointer with efi_phys_to_kva() and not bother with efi_enter(). What I gathered was that your initial instinct was the latter since efi_enter() is heavyweight, but that the former would probably be better here because:

I can think only of one specific situation where DMAP would fail. Namely, if EFI_CONFIGURATION_TABLE element overflows into the next page.

I will be happy if you just remove efi_phys_to_kva() in the scope of this patch.

I'll update this here in a little bit and post a follow-up to remove both the ioctl and this function.

[kib]

In D27669#620982, @kevans wrote:

I'll update this here in a little bit and post a follow-up to remove both the ioctl and this function.

Are you sure that ioctl is not used ? The address returned is physical, and I can easily see usermode tools opening /dev/mem to read corresponding table. In spirit of acpidump(8).

[kevans]

In D27669#620996, @kib wrote:

Are you sure that ioctl is not used ? The address returned is physical, and I can easily see usermode tools opening /dev/mem to read corresponding table. In spirit of acpidump(8).

Right now any use immediately panics the system because efi_get_table() dereferences the config table pointer

[kevans]

This is now staged as two commits, but the other one didn't necessarily seem worth its own review:

commit cf6254d2075ee7f97ca433cf6b63373547dd0e02 (HEAD -> kbsd/efi)
Author: Kyle Evans <[e-mail]>
Date: Sun Dec 27 11:26:45 2020 -0600

kern: efirt: correct configuration table entry size

Each entry actually stores a native pointer, not a uint64_t quantity. While
we're here, go ahead and export the pointer as-is rather than converting it
to KVA. This may be more useful as consumers can map /dev/mem and observe
the entry.

For reference, see: sys/contrib/edk2/Include/Uefi/UefiSpec.h

Specifically, the second commit fixes the type of ct->ct_data and removes efi_phys_to_kva().

[kib]

sys/sys/efi.h

55

This way it was compat32-clean.

[kevans]

sys/sys/efi.h

55

We don't export these anywhere that compat32 would matter, AFAICT, but we'll need to introduce another type if we start to since we use this guy for table traversal and one could seemingly, with a little work, enable efirt on armv7 if u-boot made it enticing enough.

[jrtc27]

sys/sys/efi.h

55

It wasn't CHERI-clean though... :)