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