On Fri, Mar 12, 2021 at 4:02 AM Jon Nettleton via groups.io
<jon=solid-run.com@groups.io> wrote:
>
> On Thu, Mar 11, 2021 at 11:39 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 11 Mar 2021 at 23:25, Laszlo Ersek <lersek@redhat.com> wrote:
> > >
> > > Adding Ard and Leif, comments below:
> > >
> > > On 03/11/21 15:50, Laszlo Ersek wrote:
> > > > On 03/11/21 10:48, Jon Nettleton wrote:
> > >
> > > [...]
> > >
> > > >> And this is where the pointer gets remapped again and into the MMIO
> > > >> space of the nor flash.  If I remove the calls to ConvertPointer for
> > > >> the FvbProtocol I am still seeing those addresses getting remapped
> > > >> but only once and runtime works as expected.
> > > >>
> > > >> I am seeing that in
> > > >> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> > > >> &mVariableModuleGlobal->FvbInstance->* are all being converted.  It
> > > >> is possible this is a long standing bug and it just so happens that
> > > >> our configuration has caused a conflict and exposed it.
> > > >
> > > > Yes, this is curious, I noticed it too yesterday, trying to see where
> > > > the FVB protocol member function pointers were converted. I found that
> > > > OVMF's flash driver (OvmfPkg/QemuFlashFvbServicesRuntimeDxe) didn't do
> > > > it, but MdeModulePkg/Universal/Variable/RuntimeDxe did. That was
> > > > certainly strange, as the variable driver is a consumer of the
> > > > protocol (not the producer thereof), so I'd say it has no business
> > > > poking new values into the protocol interface structure.
> > >
> > > [...]
> > >
> > > > ... Strangely, the other flash (FVB) driver in edk2,
> > > > ArmPlatformPkg/Drivers/NorFlashDxe, *does* perform the conversion
> > > > itself! See NorFlashVirtualNotifyEvent().
> > > >
> > > > I don't understand that. Is it possible that, with
> > > > "ArmPlatformPkg/Drivers/NorFlashDxe" too, the conversion happens
> > > > *twice*, but (at least) one of those mappings is "identity"?
> > >
> > > Confirmed.
> > >
> > > I had to write some elaborate debug patches for determining this,
> > > because in ArmVirtQemu, I cannot produce DEBUG output from the
> > > SetVirtualAddressMap() notification functions. So here's the approach I
> > > took:
> > >
> > > (1) Introduce a new GUID-ed HOB structure in MdeModulePkg. The structure
> > > itself lives in reserved memory, but its address is exposed in a GUID-ed
> > > HOB. The structure is named FVB_ADDRESS_LIST, and it has the following
> > > fields:
> > >
> > >   - signature ("FVBADRLS" -- FVB Address List)
> > >   - 16 entries of:
> > >     - owner signature [what driver set this entry]
> > >     - address
> > >   - number of entries used (aka next entry to fill)
> > >
> > > (2) In PlatformPei, allocate and initialize this structure (in reserved
> > > memory), and expose its address via the GUID-ed HOB. Furthermore,
> > > produce a log message with the allocation address.
> > >
> > > (3) In NorFlashDxe, look up the structure via the GUID-ed HOB, in the
> > > entry point function; remember the address in a global variable. In the
> > > SetVirtualAddressMap() handler function, treat the conversion of the
> > > "GetPhysicalAddress" FVB member function specially: via the global
> > > variable pointer to FVB_ADDRESS_LIST in reserved memory, save both the
> > > physical (original) and the virtual (converted) address of the
> > > "GetPhysicalAddress" FVB member function, in new entries. As owner
> > > signature in both entries, use "NORFLASH".
> > >
> > > (4) In the runtime DXE variable driver, do the exact same thing, just
> > > use a different "owner signature" -- "VARIABLE".
> > >
> > > (5) Once the guest is up and running, run "efibootmgr --delete-timeout"
> > > at a root prompt in the guest, deleting the existent "Timeout" UEFI
> > > non-volatile variable, for verifying that the runtime variable (write)
> > > service is functional.
> > >
> > > (6) Using the log message from point (2):
> > >
> > > > PlatformPeim: FvbAddressList @ 13FEC9000
> > >
> > > hexdump the guest memory containing the FVB_ADDRESS_LIST, as follows:
> > >
> > > > $ virsh qemu-monitor-command aavmf.rhel7.registered --hmp xp  /268cb 0x13FEC9000
> > >
> > > Ccomments to the right of the hexdump:
> > >
> > > > 000000013fec9000: 'F' 'V' 'B' 'A' 'D' 'R' 'L' 'S'                         <- structure signature: FVBADRLS
> > > > 000000013fec9008: 'N' 'O' 'R' 'F' 'L' 'A' 'S' 'H'                         <- entry[0], signature: NORFLASH
> > > > 000000013fec9010: 'T' ' ' '\xc6' ';' '\x01' '\x00' '\x00' '\x00'          <- entry[0], GetPhysicalAddress *physical*: 0x000000013bc62054
> > > > 000000013fec9018: 'N' 'O' 'R' 'F' 'L' 'A' 'S' 'H'                         <- entry[1], signature: NORFLASH
> > > > 000000013fec9020: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[1], GetPhysicalAddress *virtual*: 0x00000000244e2054
> > > > 000000013fec9028: 'V' 'A' 'R' 'I' 'A' 'B' 'L' 'E'                         <- entry[2], signature: VARIABLE
> > > > 000000013fec9030: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[2], GetPhysicalAddress *physical*: 0x00000000244e2054
> > > > 000000013fec9038: 'V' 'A' 'R' 'I' 'A' 'B' 'L' 'E'                         <- entry[3], signature: VARIABLE
> > > > 000000013fec9040: 'T' ' ' 'N' '$' '\x00' '\x00' '\x00' '\x00'             <- entry[3], GetPhysicalAddress *virtual*: 0x00000000244e2054
> > > > 000000013fec9048: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9050: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9058: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9060: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9068: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9070: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9078: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9080: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9088: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9090: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9098: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90a0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90a8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90b0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90b8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90c0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90c8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90d0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90d8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90e0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90e8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90f0: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec90f8: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9100: '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00' '\x00'
> > > > 000000013fec9108: '\x04' '\x00' '\x00' '\x00'                             <- number of entries used: 4
> > >
> > > This shows the following:
> > >
> > > - both NorFlashDxe and the runtime DXE variable driver converted the
> > >   FVB.GetPhysicalAddress member function,
> > >
> > > - the NorFlashDxe driver acted first, the runtime DXE variable driver
> > >   acted second,
> > >
> > > - when the runtime DXE variable driver "converted" the "physical"
> > >   address to virtual address, there was no change (and no crash!),
> > >   because the virtual address map passed in by the Linux kernel
> > >   apparently identity maps this area -- just as I guessed.
> > >
> > > So we definitely have a bug (only Linux's page tables save us from the
> > > crash); now the question is:
> > >
> > > Which driver is wrong to even attempt the conversion of the FVB member
> > > functions?
> > >
> > > The answer must be documented somewhere highly visible.
> > >
> > > Debug patches attached, for the record (based on commit edd46cd407ea).
> > >
> >
> > Thanks for inviting me to this party!
> >
> > So the tl;dr here is that some points get converted twice, which
> > usually is not a problem because the virtual address resulting from
> > the conversion is rarely mistaken for a physical address living in a
> > EFI_MEMORY_RUNTIME region.
> >
> > So I agree with Laszlo's assertion that the consumer of a protocol has
> > no business updating its protocol pointers, so this should definitely
> > be fixed in the core VariableRuntime driver. However, given the
> > typical nature of the variable stack, i.e., a platform specfic NOR
> > flash driver combined with the generic FTW and variable drivers, doing
> > so would likely break many out of tree platforms where the NOR flash
> > driver does not bother to update its pointers at all.
>
> Not ideal, but we could add a flag to Runtime Services and let the platform
> specify if it is remapping the FVB.  This would allow us to not break legacy
> drivers, but still easily let properly designed platforms bypass this
> behaviour.  As time progressed this could be used to for a deprecation
> warning, until it became the default handling of FVB pointer conversion.
>
Something like the attached patch possibly?