NDIS 6.20 upgrade

[jkunkee]

Using MSDN guidance, this is a naïve update of the driver from NDIS v6.1 to v6.20.

Maintaining 6.1 compatibility would require making all of these changes switch at runtime. Since Vista no longer receives security updates, this was deemed too complex and 6.1 compat has been dropped.

[jkunkee]

Smoke test (can join VPN and ping VPN server) passes on Win7 SP1 x64 and Win 10 1803 x64.

[mattock]

Merging based on a lazy-ACK and "builds fine on Windows 2016 + EWDK 10".

[cron2]

Hi,

On Tue, May 29, 2018 at 05:14:19AM -0700, Samuli Seppänen wrote:

Merging based on a lazy-ACK and "builds fine on Windows 2016 + EWDK 10".

Phew, I find this a fairly big change for a lazy-ACK...

Now if that had been "builds fine and has been fully tested across on W7 and W10" I might be less surprised...

Anyway. I've done a bit of review, and most of the changes are fairly straightforward to verify according to documentation

NdisInitializeReadWriteLock() -> NdisAllocateRWLock() NdisAcquireReadWriteLock() -> NdisAcquireRWLockRead/Write() NdisReleaseReadWriteLock() -> NdisReleaseRWLock()

-> NdisFreeRWLock() plus a new structure (pmCapabilities) and two new callback functions that currently do nothing (AdapterDirectOidRequest(), AdapterCancelDirectOidRequest()). There seems to be one catch - which mandates thorough testing on Win7: The documentation of NdisAllocateRWLock() states in regards to the "NdisHandle" parameter passed to NdisAllocateRWLock(): "Windows 8 and Windows Server 2012 and later: If the read/write lock is being allocated in DriverEntry before any NDIS handle is available, the caller may pass a NULL value for this parameter. (https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ndis/nf-ndis-ndisallocaterwlock) we do that in DriverEntry, we have no NDIS handle, but is this allowed on *Win7*? Where can we find out about this? gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany gert@greenie.muc.de

[selvanair]

There seems to be one catch - which mandates thorough testing on Win7: The documentation of NdisAllocateRWLock() states in regards to the "NdisHandle" parameter passed to NdisAllocateRWLock():

"Windows 8 and Windows Server 2012 and later: If the read/write lock is being allocated in DriverEntry before any NDIS handle is available, the caller may pass a NULL value for this parameter.

(https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ndis/nf-ndis-ndisallocaterwlock)

we do that in DriverEntry, we have no NDIS handle, but is this allowed on Win7? Where can we find out about this?

I think just "testing" on Windows 7 is not good enough to ensure the correctness of such usages. As per a related discussion on OSR online (https://www.osronline.com/ShowThread.cfm?link=253436 especially replies 6 and 9 ), Windows 7 could dereference this pointer (though not always, and a NULL could appear to work). So we should not assume its safe to pass a NULL handle here.

P.S. Sorry, I haven't found time for a proper review of these patches --- my unfamiliarity with the API makes it a time consuming task and the clock has been running too fast here lately...

[mattock]

@cron2 in retrospect I fully concur with your statement about my lazy-ACK here. I realized that yesterday when I moved to the NDIS 6.30 PR and noticed it had had some issues.

So, to avoid "I guess this is ok" reviews I ended up setting up a whole bunch of test instances (Windows Server 2008 R2, 2012 R2, 2016) and will push the guys at the office to get Windows 7 and 10 instances setup. The next step is extend on the test automation for Windows we have now (i.e. openvpn-windows-test) and allow running the tests against all of the Windows instances in automated fashion. I believe Powershell remoting (via WinRM or SSH) would do the trick.

[jkunkee]

I chose this design because a) I missed that version-based requirement and b) I didn't want to restructure the function enough to perform cleanup properly with the actually-required ordering. I'll start working on that.

[jkunkee]

The fix for the RWLock allocation has been pushed to the 6.30 PR. If you would like it separate, I can do that too.

[jkunkee]

New PR: #59