Skip to content

Commit

Permalink
Fix fundamental misunderstanding of how HID I/O reports work on macOS
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
Greg Domzalski committed May 7, 2024
1 parent 9ceeb6a commit b8eefbc
Show file tree
Hide file tree
Showing 2 changed files with 66 additions and 49 deletions.
114 changes: 65 additions & 49 deletions Yubico.Core/src/Yubico/Core/Devices/Hid/MacOSHidIOReportConnection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<byte[]> _reportsQueue;
private GCHandle _pinnedReportsQueue;

/// <summary>
/// The correct size, in bytes, for the data buffer to be transmitted to the device.
Expand Down Expand Up @@ -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<byte[]>();
_pinnedReportsQueue = GCHandle.Alloc(_reportsQueue);

SetupConnection();

InputReportSize = IOKitHelpers.GetIntPropertyValue(_deviceHandle, IOKitHidConstants.MaxInputReportSize);
Expand Down Expand Up @@ -124,7 +134,7 @@ private void SetupConnection()
_readBuffer,
_readBuffer.Length,
reportCallback,
GCHandle.ToIntPtr(_readHandle));
GCHandle.ToIntPtr(_pinnedReportsQueue));

IntPtr callback = Marshal.GetFunctionPointerForDelegate<IOHIDCallback>(RemovalCallback);
IOHIDDeviceRegisterRemovalCallback(_deviceHandle, callback, _deviceHandle);
Expand All @@ -150,49 +160,50 @@ private void SetupConnection()
/// </exception>
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;
}

/// <summary>
Expand Down Expand Up @@ -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(
Expand All @@ -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<byte[]>)GCHandle.FromIntPtr(context).Target;
reportsQueue.Enqueue(report);
}

/// <summary>
Expand Down Expand Up @@ -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);
Expand Down
1 change: 1 addition & 0 deletions Yubico.Core/src/Yubico/PlatformInterop/macOS/IOKitFramework/IOKitHidConstants.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit b8eefbc

Please sign in to comment.