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 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>
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>
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>
While the Wintun driver is typically released only at <major>.<minor>
milestones, the wintun.dll did see some intermediate releases.
To make MSI logic correctly decide to upgrade local wintun.dll file, the
version inscribed in wintun.dll file resources should increment in those
intermediate releases. MSI ignores file timestamps. One could use
REINSTALLMODE=emus Installer property to force copying wintun.dll when
its version doesn't change. But REINSTALLMODE applies to all files in
the MSI session and would be an additional requirement when authoring
MSI packages with wintun.dll.
Bumping only the final ZIP filename version is not sufficient.
Therefore, a <major>.<minor> or <major>.<minor>.<build> versioning is
introduced. Furthermore, we no longer distinguish between WintunVersion
and WintunVersionStr. All our releases used strictly numeric
<major>.<minor> notation, making WintunVersion and WintunVersionStr
always the same.
When the driver didn't change, just bump the version in wintun.proj and
run `msbuild wintun.proj /t:Zip` to rebuild the wintun.dll and make the
new ZIP file.
Signed-off-by: Simon Rozman <simon@rozman.si>
Windows 7 doesn't like our trick of sticking MDLs into the NBL context
area. So we do things more traditionally, by allocating an MDL with
IoAllocateMdl and freeing it with IoFreeMdl. This means that we have to
keep track of the MDL between allocation and free time, and we don't
have any more miniport reserved pointers left in the NBL. So instead we
walk the MdlChain field of the first NB, and free the one that has an
address living inside of the non-partial MDL.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Providing the DataOffset member of the NBL allocation function or
setting that member in the NB header indicates to NDIS not only that the
data starts at that offset, but that there's that amount of space
*available for it to use as it wants* before that offset. This meant
that NDIS was allowed to scribble data before the packet.
This was bounded by the size of the ring, so there was never any risk of
memory corruption, and since the ring is shared by userspace as well as
the rest of the kernel, we've always taken care of reading from it
closely, checking all values, and erroring out on corruption of the
ring. So, if NDIS wrote before the first packet, this would wind up
corrupting the RingTail and Alertable fields of the ring. The receiver
thread would then notice this, error out, and set the RingHead to
MAXULONG on its way out the door, so that userspace can detect it. And
indeed wintun.dll then started returning EOF from its write function.
Mostly this was not an issue, because we're not expecting for data to be
pushed on the head of a packet on ingress. But WSL2's Hyper-V driver is
actually pushing an ethernet header onto the front of the packet before
passing it off to Linux. Practically speaking, this manifested itself in
the RingTail and Alertable fields having Linux's MAC address! And then
the adapter would be EOF'd. This was reported as happening after WSL2
sends the *first* packet, but not others, which makes sense, because it
has to be at the beginning in order to corrupt those fields.
This fixes the problem by simply using a new MDL for the span we want,
instead of using the misunderstood DataOffset field. In order to not
need to keep track of memory allocations, we allocate the MDL as part of
the NBL's context area. And in order to avoid additional mappings, we
use IoBuildPartialMdl, which returns an MDL_PARTIAL, which does not have
an additional mapping that needs to be freed or unmapped.
After making this change, WSL2 no longer appears to halt the adapter,
and all works well.
Fixes: be8d2cb ("Avoid allocating second MDL")
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Otherwise the build fails at odd hours of the day.
Signed-off-by: Stefan Rinkes <stefan.rinkes@gmail.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
In theory, the compiler could reload PacketSize after the bounds check
but before it's passed to NdisAllocateNetBufferAndNetBufferList. In
practice, it's not actually doing that, but better safe than sorry.
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
