On 09/05/14 17:42, Jordan Justen wrote: > On Fri, Sep 5, 2014 at 2:03 AM, Laszlo Ersek <ler...@redhat.com> wrote: >> On 09/05/14 10:52, Laszlo Ersek wrote: >>> On 08/26/14 02:12, Jordan Justen wrote: >>>> On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <ler...@redhat.com> wrote: >>>>> On 08/26/14 00:24, Jordan Justen wrote: >>>>>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <ler...@redhat.com> wrote: >>>>>>> Recent changes in the QEMU ACPI table generator have shown that our >>>>>>> limited client for that interface is insufficient and/or brittle. >>>>>>> >>>>>>> Implement the full interface utilizing OrderedCollectionLib for >>>>>>> addressing >>>>>>> fw_cfg blobs by name. >>>>>>> >>>>>>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>>>>>> client, because: >>>>>>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable >>>>>>> short >>>>>>> of a complete ACPI parser, >>>>>>> - and it is fully sufficient to install the RSD PTR as an EFI >>>>>>> configuration table for the guest OS to see everything. >>>>>>> >>>>>>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>>>>>> restrictive convenience; let's stop using it. >>>>>> >>>>>> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >>>>>> >>>>>> Is this required? >>>>> >>>>> Depends on how good (how accurate) ACPI tables you want to have in your >>>>> VMs. For a few qemu releases now, qemu has been the "master" of ACPI >>>>> payload generation. SeaBIOS too has kept its own builtin tables, yes, >>>>> but when the QEMU generated payload is available, it interprets this >>>>> linker/loader script just the same. >>>>> >>>>> If you want PCI hotplug, pvpanic support; or, going forward, memory >>>>> hotplug, then yes, those things need ACPI support, and the ACPI stuff >>>>> for them is now developed in QEMU (and dynamically generated by it, >>>>> dependent on the actual hardware config). >>>>> >>>>>> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >>>>>> have two drivers that want to install the pointer. >>>>> >>>>> Yes, I checked that. The UEFI spec says that whenever you install a >>>>> configuration table that's already present (by GUID), the reference is >>>>> updated. >>>>> >>>>> InstallConfigurationTable() >>>>> >>>>> [...] >>>>> * If Guid is present in the System Table, and Table is not NULL, then >>>>> the (Guid, Table) pair is updated with the new Table value. >>>>> [...] >>>>> >>>>> For this reason, the first thing InstallAllQemuLinkedTables() does is >>>>> check, with EfiGetSystemConfigurationTable(), for the presence of any of >>>>> the ACPI GUIDs in the config table. >>>>> >>>>> There's no "absolute" cure against another driver in OVMF just grabbing >>>>> EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then >>>>> completely hide the tables installed by this patchset, because >>>>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's >>>>> RSD PTR's address in the UEFI config table with its own). >>>>> >>>>> But ACPI tables are not randomly installed in OVMF, we keep it >>>>> centralized in AcpiTableDxe. >>>> >>>> I don't agree with this statement. Rather, I would say that OVMF >>>> follows the EDK II method of publishing tables, which means >>>> EFI_ACPI_TABLE_PROTOCOL. >>>> >>>> In the past we were a good little sample platform, and provided the >>>> ACPI tables directly. Well, that is less and less the case. But, is it >>>> a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? >>>> >>>> What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which >>>> uses EFI_ACPI_TABLE_PROTOCOL? >>>> >>>> Do we need to keep monitoring which EDK II drivers decide to use >>>> EFI_ACPI_TABLE_PROTOCOL, or can we find a way to make these work >>>> together? If the answer is no, then I wonder if this is something that >>>> UEFI or EDK II needs to consider. >>> >>> I think I might have found a solution -- at this point it's just an >>> idea, but I want to run it by you guys first before I start implementing >>> it. The idea is based on this patchset, in an incremental fashion. >>> >>> Where the current code calls gBS->InstallConfigurationTable(), we shall >>> replace that call with further processing. >>> >>> Michael has proposed before to check the *targets* of all the pointer >>> commands. Namely, a pointed-to "something" in the blob that hosts it is >>> with a good chance an ACPI table. Not guaranteed, but likely. >>> >>> Now, after all is done by the current patchset *except* linking the RSDP >>> into the UEFI config table, the payload we have in AcpiNVS memory is a >>> nicely cross-linked, checksummed forest of ACPI stuff. We re-scan the >>> QEMU commands. We ignore everything different from AddPointer. When an >>> AddPointer call is found, the following statements are true: >>> >>> (a) the pointer field in the blob that the AddPointer call would >>> originally patch *now* contains a valid, absolute physical address. >>> >>> (b) the pointed-to byte-array may or may not be an ACPI table. It could >>> be an ACPI table, or it could be some funny area that QEMU has >>> preallocated, like the target of the TCPA.LASA field, or the target of >>> the BGRT.ImageAddress field, or something else. >>> >>> (c) *if* the pointed-to byte-array is an ACPI table, then we'll have it >>> already checksummed, since we're past the first complete pass of the >>> processing. >>> >>> So the idea is, look at the target area, >>> - determine if the remaining size in that blob (the pointed-to blob) >>> could still contain an ACPI table header, >>> - if so, check the presumed "length" field in that header, and see if >>> it's self-consistent (ie. >= sizeof(header), and <= remaining size in >>> blob) >>> - if so, run a checksum on the bytes that presumably constitute the >>> ACPI table >>> - if it sums to zero, we attempt to install the target area with >>> EFI_ACPI_TABLE_PROTOCOL, otherwise we leave it alone. >>> >>> Now, EFI_ACPI_TABLE_PROTOCOL makes deep copies of the tables it gets, >>> but we'll leave the entire forest that we've build during the first >>> processing in place. Why? Because this way all the stuff that *didn't* >>> look like an ACPI table during the procedure above will remain in place, >>> and the pointers *to* them will remain valid, in those ACPI table copies >>> that EFI_ACPI_TABLE_PROTOCOL creates. >>> >>> For example, consider a BGRT table, where QEMU places both the BGRT in >>> the blob, and the image data right after it (and sets BGRT.ImageAddress >>> to the relative address inside the blob where the image data starts). >>> The above procedure will result in: >>> >>> - the BGRT table itself being allocated twice, once by our first pass, >>> and once by EFI_ACPI_TABLE_PROTOCOL itself (from the second pass). The >>> first instance will be leaked (it won't be reachable from the system >>> table that EFI_ACPI_TABLE_PROTOCOL now manages), but it's not grave. >>> >>> - the BGRT image data will be allocated only once, from our first pass, >>> and it will be referenced *both* from our first-pass BGRT table (which >>> is irrelevant) *and* from the BGRT copy that EFI_ACPI_TABLE_PROTOCOL >>> installs (which is relevant). >>> >>> This approach would leak a few pages of AcpiNVS memory, and it has a >>> slight chance to call EFI_ACPI_TABLE_PROTOCOL.InstallTable() with >>> garbage (that has a valid-looking Length field and checksums to zero). >>> But it would remain compatible with EFI_ACPI_TABLE_PROTOCOL, and it >>> wouldn't miss anything from QEMU's payload that in fact *is* an ACPI table. >> >> In addition, I could tighten the Length + checksum validation with >> ACPI_BUILD_APPNAME6 and ACPI_BUILD_APPNAME4 checks, according to qemu's >> build_header() function -- if Michael agrees that these are stable. IOW, >> the OEMID would have to be "BOCHS ", and the first four bytes of >> OEMTableID would have to be "BXPC". I think these four checks together >> are pretty strong: a static check for a *10-byte* signature (in effect), >> and a dynamic check for length + checksum. >> >> I think this should be acceptable. > > It sounds like it would solve my concern. > > It doesn't sound like it would be a huge amount of extra effort or code. > > The part about having to identify tables from the blob is not great, > but it does seem like your plan is unlikely to mistakenly identify a > non-table as a table. > > Is it possible to get more confirmation that the few wasted pages > would remain small. I guess maybe the easiest argument to this would > be that QEMU is not going to have a large AcpiNVS blob in the first > place because it would always mean taking some RAM from the OS.
I've written the code such that it tracks, per fw_cfg blob, if the blob had any pointers into it that turned out to be non-tables. If it didn't, then the blob is released in the end. In my current testing, there are two blobs in total, /etc/acpi/rsdp and /etc/acpi/tables, and both are released. Hence no pages are wasted at all. In general, as far as I understand, QEMU developers don't intend to push any blobs > 64K, and an idea was raised to keep a minimal number of tables per blob. So this should limit the leakage even if a few blobs remain with "opaque" contents in them. I'll go through the patches again and post them soon. Thanks Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
