[vbox-dev] PATCH: Implement support for multiple custom ACPI tables

[Michal Necasek]

     Hi,

  Sorry for the delay.

  You're absolutely right, the acpiR3SetupCust() function does a lot of 
things but acpiR3PhysCopy() is the only useful bit it does.

  The only objection I have then is the following: Do we really need 
three different names for one thing? SLICTable, CustomTable, 
CustomTable0 for the same thing seems like an overkill.

     Cheers,
        Michal

On 9/7/2018 10:22 AM, Canardos . wrote:
> Hi Michal,
> 
> Thanks for taking a look.
> 
> I agree that the behavior should not change and I don't believe it does. 
> I removed the call to acpiR3PrepareHeader (as well as the surrounding 
> lines) because it was dead code. Looking at the prior acpiR3SetupCust 
> function:
> 
> /** Custom Description Table */
> static void acpiR3SetupCust(ACPIState *pThis, RTGCPHYS32 addr)
> {
>      ACPITBLCUST cust;
> 
>      /* First the ACPI version 1 version of the structure. */
>      memset(&cust, 0, sizeof(cust));
>      acpiR3PrepareHeader(pThis, &cust.header, "CUST", sizeof(cust), 1);
> 
>      memcpy(cust.header.au8OemTabId, pThis->au8OemTabId, 8);
>      cust.header.u32OemRevision = RT_H2LE_U32(pThis->u32OemRevision);
>      cust.header.u8Checksum = acpiR3Checksum((uint8_t *)&cust, 
> sizeof(cust));
> 
>      acpiR3PhysCopy(pThis, addr, pThis->pu8CustBin, pThis->cbCustBin);
> }
> 
> One can see that the cust structure is allocated locally on the first 
> line, setup, but then discarded when the function exits without ever 
> being used. The only code in the function that does anything is the 
> final line.
> 
> The custom table header that is actually used is the one provided by the 
> user, which is loaded and present in the table itself 
> (pThis->pu8CustBin). This behavior has been retained in the updated 
> function:
> 
> /** Custom Description Table */
> static void acpiR3SetupCust(ACPIState *pThis, RTGCPHYS32 addr, uint8_t 
> custTableIdx)
> {
>      Assert(custTableIdx < MAX_CUST_TABLES);
>      acpiR3PhysCopy(pThis, addr, pThis->pu8CustBin[custTableIdx], 
> pThis->cbCustBin[custTableIdx]);
> }
> 
> With respect to the incoming data, which part did you notice was copied 
> differently? The intention is definitely to retain the existing behavior 
> so if there's a difference there, it's unintended.
> 
> In regard to use cases, one example is the requirement to now pass 
> through both SLIC and MSDM tables when using (legally) using a Windows 
> 10 OEM edition as a guest on the machine that it was licensed on (this 
> was the motivation for the original custom table mechanism for SLIC 
> passthrough for Windows 7/8 - see 
> https://www.virtualbox.org/ticket/9231). Reducing the detection surface 
> for dynamic analysis of malware that actively detects sandboxes is another.
> 
> Regards,
> 
> On Tue, Sep 4, 2018 at 10:45 PM Michal Necasek 
> <michal.necasek at oracle.com <mailto:michal.necasek at oracle.com>> wrote:
> 
> 
>          Hi,
> 
>        Thanks for the patch! I quickly looked over it and it looks to me
>     that
>     the behavior of existing VMs with the
>     VBoxInternal/Devices/acpi/0/Config/CustomTable keys set will subtly
>     change because the call to
> 
>     acpiR3PrepareHeader(pThis, &cust.header, "CUST", sizeof(cust), 1);
> 
>     is no longer done and the incoming data is copied a bit differently.
>     Can
>     you explain why that's not done or why it can't cause any problems?
> 
>        Wouldn't it be better to keep the existing behavior for CustomTable
>     and just add CustomTable1/2/3 with slightly different behavior?
> 
>        Also, do you have a concrete example of ACPI tables that one might
>     want to pass to a VM this way?
> 
> 
>           Regards,
>             Michal
> 
>     On 8/24/2018 9:14 PM, Canardos . wrote:
>      > Hi Devs,
>      >
>      > This patch expands on work done several years ago, extending
>     support for
>      > custom ACPI tables from a single table to four tables, and
>     potentially N
>      > tables in future.
>      >
>      > The changes assist malware researchers to better emulate a physical
>      > machine (although increasing the aggregate allowable table size will
>      > assist further) as well as those with valid OEM licenses having
>     issues
>      > property passing through both SLIC and license tables.
>      >
>      > I limited the implementation to four custom tables given current
>     VBox
>      > limitations on the aggregate size of ACPI tables, but the code
>     changes
>      > support N tables.
>      >
>      > The new configuration keys are "CustomTable0..3". Legacy behavior
>     has
>      > been maintained, with any existing config entries for
>     "CustomTable", or
>      > "SLICTable" being used in the absence of a "CustomTable0" entry.
>      >
>      > The changes have been tested on Debian 9.5 (kernel 4.9.0-7/amd64)
>     and
>      > Manjaro (kernel 4.14.65-1/amd64) hosts with 32 and 64-bit Linux
>     guests.
>      >
>      > I've endeavored to reflect the code style of the file I was
>     working in,
>      > please let me know if anything is amiss.
>      >
>      > All attached code is released under the MIT License.
>      >
>      >
>      >
>      > _______________________________________________
>      > vbox-dev mailing list
>      > vbox-dev at virtualbox.org <mailto:vbox-dev at virtualbox.org>
>      > https://www.virtualbox.org/mailman/listinfo/vbox-dev
>      >
> 
>     _______________________________________________
>     vbox-dev mailing list
>     vbox-dev at virtualbox.org <mailto:vbox-dev at virtualbox.org>
>     https://www.virtualbox.org/mailman/listinfo/vbox-dev
>