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

Sat Sep 22 09:42:45 GMT 2018

Hi Michal,

>Do we really need three different names for one thing? SLICTable,
CustomTable, CustomTable0 for the same thing seems like an overkill.

I agree. Ideally it would just be CustomTable0, CustomTable1,
CustomTable2.....and the code currently supports that. I had kept
SLICTable/CustomTable as aliases for CustomTable0 to maintain backward
compatibility, expecting that at some point in the future they would likely
be removed.

It's easy enough to remove the SLICTable/CustomTable aliases now if
preferred. I would leave that decision to the devs, since it would be a
breaking change for users currently using those config params.

Regards,

On Tue, Sep 18, 2018 at 3:56 PM Michal Necasek <michal.necasek at oracle.com>
wrote:

>
>      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
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.virtualbox.org/pipermail/vbox-dev/attachments/20180922/4b83f268/attachment.html>