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.neca...@oracle.com <mailto:michal.neca...@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@virtualbox.org <mailto:vbox-dev@virtualbox.org>
     > https://www.virtualbox.org/mailman/listinfo/vbox-dev
     >

    _______________________________________________
    vbox-dev mailing list
    vbox-dev@virtualbox.org <mailto:vbox-dev@virtualbox.org>
    https://www.virtualbox.org/mailman/listinfo/vbox-dev


_______________________________________________
vbox-dev mailing list
vbox-dev@virtualbox.org
https://www.virtualbox.org/mailman/listinfo/vbox-dev

Reply via email to