There's only one handle that's likely to be open in a long lived way:
the tun registration handle. So we can force that closed automatically
when the device is about to close, if it's been improperly left open.
Other handles will indeed hold up closing, but if those exist, they're a
sign of a larger bug elsewhere that should be addressed. On the other
hand, tun registration handles might legitimately be open during driver
upgrades. This also saves us the trouble of dereferencing a freed
FileObject as in the general case.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Not discouraging userspace from skipping checking IP packets seems like
a bad thing, but they skip it anyway, so at least avoid the DoS due to
API misuse.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
CodeQL with Windows-Driver-Developer-Supplemental-Tools suggests the
ExAllocatePoolWithTag() should no longer be used. The Static Tools Logo
Test in HLK spots this in the DVL log and fails.
Signed-off-by: Simon Rozman <simon@rozman.si>
SDV is allergic to code analysis. So, when we're doing SDV (SDVHacks is
"true"), we need to turn the code analysis off.
Signed-off-by: Simon Rozman <simon@rozman.si>
SDV is using own CL.EXE which returns error code 2 when code analysis
is turned on. However, we need code analysis results for DVL.
While we could use a new "ReleaseSDV" configuration, we don't really
require limited code analysis in Release builds, as long as we address
all full code analysis warnings in Debug builds.
To make DVL happier, an intermediate Release build was injected with
code analysis turned on.
Signed-off-by: Simon Rozman <simon@rozman.si>
Makes builds more reproducable, as we can do our next release using the
EWDK, an all-in-one ISO of build tools from Microsoft.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
The initial adapter state (including media connection) is provided by
the NDIS_MINIPORT_ADAPTER_GENERAL_ATTRIBUTES.
Additional NdisMIndicateStatusEx() call seems excessive.
Signed-off-by: Simon Rozman <simon@rozman.si>
All was well with NetSetupAnticipatedInstanceId, until a bug crept into
recent Windows builds that caused old GUIDs not to be properly removed,
resulting in subsequent adapter creations to fail, because NetSetup
AnticipatedInstanceId considers it fatal when the target GUID
already exists, even if in diminished form.
The initial solution was to detect cruft, and then steal a
TrustedInstaller token and sleuth around the registry cleaning things
up. The horror!
Uncomfortable with this, I reopened IDA and had a look around with fresh
eyes, three years after the original discovery of NetSetupAnticipated
InstanceId. There, I found some interesting behavior in
NetSetupSvcDeviceManager::InstallNetworkInterfaces, which amounts to
something like:
if (IsSet("RetiredNetCfgInstanceId") {
if (IsSet("NetSetupAnticipatedInstanceId")
DeleteAdapter(GetValue("RetiredNetCfgInstanceId"));
else
Set("NetSetupAnticipatedInstanceId", GetValue("RetiredNetCfgInstanceId"));
Delete("RetiredNetCfgInstanceId");
}
CreateAdapter = TRUE;
if (IsSet("NetSetupAnticipatedInstanceId")) {
Guid = GetValue("NetSetupAnticipatedInstanceId");
if (AdapterAlreadyExists(Guid))
CreateAdapter = FALSE;
else
SetGuidOfNewAdapter(Guid);
Delete("NetSetupAnticipatedInstanceId");
} else if (IsSet("SuggestedInstanceId")) {
Guid = GetValue("SuggestedInstanceId");
if (!AdapterAlreadyExists(Guid))
SetGuidOfNewAdapter(Guid);
Delete("SuggestedInstanceId");
}
Thus, one appealing strategy would be to set both NetSetupAnticipated
InstanceId and RetiredInstanceId to the same value, and let the service
handle deleting the old one for us before creating the new one.
However, the cleanup of the old adapter winds up being quasi-
asynchronous, and thus we still wind up in the CreateAdapter = FALSE
case.
So, the remaining strategy is to simply use SuggestedInstanceId instead.
This has the behavior that if there's an adapter already in use, it'll
use a new random GUID. The result is that adapter creation won't fail.
That's not great, but the docs have always made it clear that
"requested" is a best-effort sort of thing. Plus, hopefully the creation
of the new adapter will help nudge the bug a bit and cleanup the old
cruft. In some ways, transitioning from our old strategy of "cudgel the
registry until we get the GUID we want" to "ask politely and accept no
for an answer" is a disappointing regression in functionality. But it
also means we don't need to keep crazy token stealing code around, or
fish around in the registry dangerously. This probably also increases
the likelihood that an adapter will be created during edge cases, which
means fewer errors for users, which could be a good thing. On the
downside, we have the perpetual tensions caused by a system that now
"fails open" instead of "fails closed". But so it goes in Windows land.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Prior to the conversion, LastError is ERROR_SUCCESS, so move the logging
to be after the conversion.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
There's no longer a need to do this for every API call. This only exists
now for the pnp guid reuse workaround hack.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
IoAllocateMdl allocates a different size structure depending on the
bottom in-page bits of the address. By passing null, it assumes that the
address is aligned within the page, which it might not be. Fix this by
passing the eventual virtual address to the allocation function so that
the right amount is always allocated.
Reported-by: Oleksandr Muzychuk <oleksandr.muzychuk@apriorit.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Should NDIS version check fail the DriverEntry() bailed out without
releasing the TunDispatchSecurityDescriptor.
Signed-off-by: Simon Rozman <simon@rozman.si>
