From b8eefbc7970813d370f85603068a06d861ee7d54 Mon Sep 17 00:00:00 2001 From: Greg Domzalski Date: Tue, 7 May 2024 19:47:50 -0400 Subject: [PATCH] Fix fundamental misunderstanding of how HID I/O reports work on macOS This addresses a bug where if a YubiKey command would result in multiple input reports being generated, we would get the callbacks but drop the results on the floor. This would generally result in the runloop timing out because we already had received the report via the callback but had no way of caching it. --- .../Devices/Hid/MacOSHidIOReportConnection.cs | 114 ++++++++++-------- .../macOS/IOKitFramework/IOKitHidConstants.cs | 1 + 2 files changed, 66 insertions(+), 49 deletions(-) diff --git a/Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs b/Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs index efc0d26b8..57413fc5c 100644 --- a/Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs +++ b/Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs @@ -13,8 +13,8 @@ // limitations under the License. using System; +using System.Collections.Concurrent; using System.Globalization; -using System.Linq; using System.Runtime.InteropServices; using System.Text; using Yubico.Core.Buffers; @@ -35,8 +35,12 @@ internal sealed class MacOSHidIOReportConnection : IHidConnection private readonly MacOSHidDevice _device; private readonly IntPtr _loopId; private readonly Logger _log = Log.GetLogger(); + private readonly byte[] _readBuffer; - private readonly GCHandle _readHandle; + private GCHandle _readHandle; + + private readonly ConcurrentQueue _reportsQueue; + private GCHandle _pinnedReportsQueue; /// /// The correct size, in bytes, for the data buffer to be transmitted to the device. @@ -67,9 +71,15 @@ public MacOSHidIOReportConnection(MacOSHidDevice device, long entryId) byte[] cstr = Encoding.UTF8.GetBytes($"fido2-loopid-{entryId}"); _loopId = CFStringCreateWithCString(IntPtr.Zero, cstr, 0); + // The following buffer must be pinned because the native function must retain a pointer (i.e. the address) _readBuffer = new byte[64]; _readHandle = GCHandle.Alloc(_readBuffer, GCHandleType.Pinned); + // This object is marshalled using a normal handle since .NET is on the other side and is capable of using + // the GCHandle (i.e. we do not need the address) + _reportsQueue = new ConcurrentQueue(); + _pinnedReportsQueue = GCHandle.Alloc(_reportsQueue); + SetupConnection(); InputReportSize = IOKitHelpers.GetIntPropertyValue(_deviceHandle, IOKitHidConstants.MaxInputReportSize); @@ -124,7 +134,7 @@ private void SetupConnection() _readBuffer, _readBuffer.Length, reportCallback, - GCHandle.ToIntPtr(_readHandle)); + GCHandle.ToIntPtr(_pinnedReportsQueue)); IntPtr callback = Marshal.GetFunctionPointerForDelegate(RemovalCallback); IOHIDDeviceRegisterRemovalCallback(_deviceHandle, callback, _deviceHandle); @@ -150,49 +160,50 @@ private void SetupConnection() /// public byte[] GetReport() { - try + if (_reportsQueue.TryDequeue(out byte[] report)) { - IntPtr runLoop = CFRunLoopGetCurrent(); + // If there's already a report in the queue (i.e. the callback beat us to calling GetReport) return + // that one immediately. + _log.SensitiveLogInformation( + "GetReport returned buffer: {Report}", + Hex.BytesToHex(report)); - IOHIDDeviceScheduleWithRunLoop(_deviceHandle, runLoop, _loopId); + return report; + } - // The YubiKey has a reclaim timeout of 3 seconds. This can cause the SDK some trouble if we just - // switched out of a different USB interface (like Keyboard or CCID). We previously used a fairly - // tight timeout of 4 seconds, but that seemed to not always work. 6 seconds (double the timeout) - // seems like a more reasonable timeout for the operating system. - int runLoopResult = CFRunLoopRunInMode(_loopId, 6, true); + // Otherwise start up the IO runloop and see if we find more reports to pick up. + IntPtr runLoop = CFRunLoopGetCurrent(); - _device.LogDeviceAccessTime(); + IOHIDDeviceScheduleWithRunLoop(_deviceHandle, runLoop, _loopId); - if (runLoopResult != kCFRunLoopRunHandledSource) - { - throw new PlatformApiException( - string.Format( - CultureInfo.CurrentCulture, - ExceptionMessages.WrongIOKitRunLoopMode, - runLoopResult)); - } + // The YubiKey has a reclaim timeout of 3 seconds. This can cause the SDK some trouble if we just + // switched out of a different USB interface (like Keyboard or CCID). We previously used a fairly + // tight timeout of 4 seconds, but that seemed to not always work. 6 seconds (double the timeout) + // seems like a more reasonable timeout for the operating system. + int runLoopResult = CFRunLoopRunInMode(_loopId, 6, true); - IOHIDDeviceUnscheduleFromRunLoop(_deviceHandle, runLoop, _loopId); - - _log.SensitiveLogInformation( - "GetReport returned buffer: {Report}", - Hex.BytesToHex(_readBuffer)); + _device.LogDeviceAccessTime(); - // Return a copy of the report - return _readBuffer.ToArray(); - } - finally + if (runLoopResult != kCFRunLoopRunHandledSource) { - IOHIDDeviceRegisterInputReportCallback( - _deviceHandle, - _readBuffer, - _readBuffer.Length, - IntPtr.Zero, - IntPtr.Zero); - - IOHIDDeviceRegisterRemovalCallback(_deviceHandle, IntPtr.Zero, IntPtr.Zero); + throw new PlatformApiException( + string.Format( + CultureInfo.CurrentCulture, + ExceptionMessages.WrongIOKitRunLoopMode, + runLoopResult)); } + + IOHIDDeviceUnscheduleFromRunLoop(_deviceHandle, runLoop, _loopId); + + // We should be guaranteed to have a report here - otherwise the runloop would have timed out + // and the PlatformApiException above would have been thrown. + _ = _reportsQueue.TryDequeue(out report); + + _log.SensitiveLogInformation( + "GetReport returned buffer: {Report}", + Hex.BytesToHex(report)); + + return report; } /// @@ -232,7 +243,7 @@ private static void ReportCallback( log.LogInformation("MacOSHidIOReportConnection.ReportCallback has been called."); - if (result != 0 || type != IOKitHidConstants.kIOHidReportTypeOutput || reportId != 0 || reportLength < 0) + if (result != 0 || type != IOKitHidConstants.kIOHidReportTypeInput || reportId != 0 || reportLength < 0) { // Something went wrong. We don't currently signal, just continue. log.LogWarning( @@ -242,19 +253,10 @@ private static void ReportCallback( type, reportId, reportLength); - - return; } - byte[] buffer = (byte[])GCHandle.FromIntPtr(context).Target; - long length = Math.Min(buffer.Length, reportLength); - log.LogInformation( - "Buffer length determined to be {Length} bytes. (buffer.Length was {BufferLength}, and reportLength was {ReportLength}", - length, - buffer.Length, - reportLength); - - Array.Copy(report, buffer, length); + var reportsQueue = (ConcurrentQueue)GCHandle.FromIntPtr(context).Target; + reportsQueue.Enqueue(report); } /// @@ -324,11 +326,25 @@ private void Dispose(bool disposing) // Dispose managed state here } + IOHIDDeviceRegisterInputReportCallback( + _deviceHandle, + _readBuffer, + _readBuffer.Length, + IntPtr.Zero, + IntPtr.Zero); + + IOHIDDeviceRegisterRemovalCallback(_deviceHandle, IntPtr.Zero, IntPtr.Zero); + if (_readHandle.IsAllocated) { _readHandle.Free(); } + if (_pinnedReportsQueue.IsAllocated) + { + _pinnedReportsQueue.Free(); + } + if (_deviceHandle != IntPtr.Zero) { _ = IOHIDDeviceClose(_deviceHandle, 0); diff --git a/Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs b/Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs index afc2a4c2c..f39866573 100644 --- a/Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs +++ b/Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs @@ -24,6 +24,7 @@ internal static class IOKitHidConstants public const string MaxInputReportSize = "MaxInputReportSize"; public const string MaxOutputReportSize = "MaxOutputReportSize"; + public const int kIOHidReportTypeInput = 0; public const int kIOHidReportTypeOutput = 1; public const int kIOHidReportTypeFeature = 2; }