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