Merge pull request #5 from scryner/feat/hardening-driver

Harden CrossOver driver startup and install flow

Author: scryner

Cargo.lock

@@ -8,7 +8,7 @@ members = [
 resolver = "2"
 
 [workspace.package]
-version = "0.3.0"
+version = "0.3.1-rc0"
 edition = "2021"
 license = "Apache-2.0"
 authors = ["Seonghwan Jeong <scryner@gmail.com>"]

Cargo.toml

@@ -188,13 +188,18 @@ install/update/uninstall automatically, and install/update/repair import the
 Wine loader override registry file as described above. The shell installer
 below is mainly for manual development and smoke testing.
 
-Install the driver next to `Steam.exe` in the target bottle and import the Wine
-loader override:
+Copy the driver next to `Steam.exe` in the target bottle:
 
 ```sh
 tools/install-driver.sh --bottle Steam
 ```
 
+To also import Wine's `hid=native,builtin` loader override:
+
+```sh
+tools/install-driver.sh --bottle Steam --override-dll
+```
+
 Useful options:
 
 ```sh
@@ -205,11 +210,11 @@ tools/install-driver.sh \
 ```
 
 The script copies `hid.dll`, backs up any existing local `hid.dll`, and
-initializes `crosspuck-driver.log`. It also writes
-`crosspuck-wine-override.reg` into the bottle and imports it with the matching
-CrossOver app, using CrossOver Preview first for preview-marked bottles and
-CrossOver first for regular bottles. It does not set guest runtime options or
-rewrite `user.reg` directly.
+initializes `crosspuck-driver.log`. By default it does not call CrossOver
+`regedit`. With `--override-dll`, it writes `crosspuck-wine-override.reg` into
+the bottle and imports it with the matching CrossOver app, using CrossOver
+Preview first for preview-marked bottles and CrossOver first for regular
+bottles. It does not set guest runtime options or rewrite `user.reg` directly.
 
 Do not install this DLL into `drive_c/windows/system32`. It is designed to live
 next to Steam and forward non-virtual HID calls to the real system HID DLL.

README.md

@@ -3,8 +3,8 @@ use crosspuck_core::hid::{
     HidFilter, HidSnapshotError, PuckSnapshot,
 };
 use crosspuck_core::protocol::{
-    session_trace_label, CollectionRole, FeatureResult, GetFeature, IdentityPayload, SetFeature,
-    SetFeatureResult, SetOutput, SetOutputResult, StatusCode, WriteReport, WriteResult,
+    session_trace_label, CollectionRole, FeatureResult, GetFeature, SetFeature, SetFeatureResult,
+    SetOutput, SetOutputResult, StatusCode, WriteReport, WriteResult,
 };
 use std::collections::BTreeMap;
 use std::fmt;
@@ -51,7 +51,6 @@ fn log_host_guest_warning(args: std::fmt::Arguments<'_>) {
 }
 
 pub(crate) trait HostBackend: Send + Sync {
-    fn identity(&self) -> &IdentityPayload;
     fn open_input_reader(&self) -> Result<Box<dyn InputReportReader>, HostHidError>;
     fn get_feature(&self, request: &GetFeature) -> FeatureResult;
     fn set_feature(&self, request: &SetFeature) -> SetFeatureResult;
@@ -79,7 +78,6 @@ pub(crate) struct HostInputReport {
 #[derive(Clone, Debug)]
 pub(crate) struct RealHostBackend {
     snapshot: Arc<Mutex<PuckSnapshot>>,
-    identity: IdentityPayload,
     main: SharedMainDevice,
     interface_devices: Arc<Mutex<BTreeMap<u8, CachedInterfaceDevice>>>,
 }
@@ -98,11 +96,10 @@ struct CachedInterfaceDevice {
 }
 
 impl RealHostBackend {
-    pub fn new(snapshot: PuckSnapshot, identity: IdentityPayload) -> Result<Self, HostHidError> {
+    pub fn new(snapshot: PuckSnapshot) -> Result<Self, HostHidError> {
         let main = Self::open_main_device(&snapshot)?;
         Ok(Self {
             snapshot: Arc::new(Mutex::new(snapshot)),
-            identity,
             main,
             interface_devices: Arc::new(Mutex::new(BTreeMap::new())),
         })
@@ -427,10 +424,6 @@ impl RealHostBackend {
 }
 
 impl HostBackend for RealHostBackend {
-    fn identity(&self) -> &IdentityPayload {
-        &self.identity
-    }
-
     fn open_input_reader(&self) -> Result<Box<dyn InputReportReader>, HostHidError> {
         let readers = self.open_input_devices();
         if readers.is_empty() {
@@ -1006,11 +999,19 @@ impl fmt::Display for HostHidError {
                 write!(f, "invalid HID interface number: {interface_number}")
             }
             Self::DeviceLockPoisoned => write!(f, "HID device lock poisoned"),
+            Self::Hid(error) if is_macos_hid_permission_error(error) => write!(
+                f,
+                "{error}; macOS denied HID access. Run CrossPuck as an app bundle and enable Privacy & Security > Input Monitoring for CrossPuck."
+            ),
             Self::Hid(error) => write!(f, "{error}"),
         }
     }
 }
 
+fn is_macos_hid_permission_error(error: &HidSnapshotError) -> bool {
+    matches!(error, HidSnapshotError::Hid(_)) && error.to_string().contains("0xE00002E2")
+}
+
 impl std::error::Error for HostHidError {
     fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
         match self {

crates/crosspuck-app/src/hid_backend.rs

@@ -350,23 +350,33 @@ fn handle_session(
             }
         };
     let identity = IdentityPayload::try_from(&snapshot)?;
-    let backend = Arc::new(with_worker_autorelease_pool(|| {
-        RealHostBackend::new(snapshot, identity)
-    })?);
-    handle_session_with_backend(
-        listeners,
-        control,
-        SessionStart {
-            session_id,
-            session_trace_id,
-            guest_runtime_overrides,
-            hello_request_id: hello_frame.header.id,
-            guest_pid: hello.guest_pid,
-        },
-        app_state,
-        backend,
-        stop,
-    )
+    let session = SessionStart {
+        session_id,
+        session_trace_id,
+        guest_runtime_overrides,
+        hello_request_id: hello_frame.header.id,
+        guest_pid: hello.guest_pid,
+    };
+    app_state.set(HostRuntimeState::PuckConnected {
+        serial: identity.serial.clone(),
+    });
+    let backend = match with_worker_autorelease_pool(|| RealHostBackend::new(snapshot)) {
+        Ok(backend) => Arc::new(backend),
+        Err(error) => {
+            let hello_ok = hello_ok_with_status(
+                StatusCode::DeviceDisconnected,
+                session_id,
+                session_trace_id,
+                identity.default_input_report_len(),
+            );
+            write_payload(control, hello_frame.header.id, &hello_ok)?;
+            return Err(error.into());
+        }
+    };
+    write_session_preamble(control, &session, &identity)?;
+    let input =
+        attach_input_for_session(listeners, stop, INPUT_ATTACH_TIMEOUT, &session, &identity)?;
+    run_attached_session(control, input, session, app_state, backend, stop, identity)
 }
 
 #[derive(Clone, Debug)]
@@ -378,11 +388,13 @@ struct SessionStart {
     guest_pid: u32,
 }
 
+#[cfg(test)]
 fn handle_session_with_backend(
     listeners: &TransportListeners,
     control: &mut ChannelStream,
     session: SessionStart,
     app_state: &AppState,
+    identity: IdentityPayload,
     backend: Arc<dyn HostBackend>,
     stop: &Arc<AtomicBool>,
 ) -> Result<(), RuntimeError> {
@@ -391,26 +403,39 @@ fn handle_session_with_backend(
         control,
         session,
         app_state,
+        identity,
         backend,
         stop,
         INPUT_ATTACH_TIMEOUT,
     )
 }
 
+#[cfg(test)]
 fn handle_session_with_backend_timeout(
     listeners: &TransportListeners,
     control: &mut ChannelStream,
     session: SessionStart,
     app_state: &AppState,
+    identity: IdentityPayload,
     backend: Arc<dyn HostBackend>,
     stop: &Arc<AtomicBool>,
     input_attach_timeout: Duration,
 ) -> Result<(), RuntimeError> {
-    let identity = backend.identity().clone();
     app_state.set(HostRuntimeState::PuckConnected {
         serial: identity.serial.clone(),
     });
 
+    write_session_preamble(control, &session, &identity)?;
+    let input =
+        attach_input_for_session(listeners, stop, input_attach_timeout, &session, &identity)?;
+    run_attached_session(control, input, session, app_state, backend, stop, identity)
+}
+
+fn write_session_preamble(
+    control: &mut ChannelStream,
+    session: &SessionStart,
+    identity: &IdentityPayload,
+) -> Result<(), RuntimeError> {
     write_payload(
         control,
         session.hello_request_id,
@@ -428,8 +453,17 @@ fn handle_session_with_backend_timeout(
             log_level.as_str()
         );
     }
-    write_payload(control, 0, &identity)?;
+    write_payload(control, 0, identity)?;
+    Ok(())
+}
 
+fn attach_input_for_session(
+    listeners: &TransportListeners,
+    stop: &Arc<AtomicBool>,
+    input_attach_timeout: Duration,
+    session: &SessionStart,
+    identity: &IdentityPayload,
+) -> Result<ChannelStream, RuntimeError> {
     let mut input = accept_input_for_session(listeners, stop, input_attach_timeout)?;
     input.set_write_timeout(Some(Duration::from_millis(250)))?;
     input.set_read_timeout(Some(Duration::from_millis(250)))?;
@@ -461,7 +495,18 @@ fn handle_session_with_backend_timeout(
             actual: attach.session_id,
         });
     }
+    Ok(input)
+}
 
+fn run_attached_session(
+    control: &mut ChannelStream,
+    input: ChannelStream,
+    session: SessionStart,
+    app_state: &AppState,
+    backend: Arc<dyn HostBackend>,
+    stop: &Arc<AtomicBool>,
+    identity: IdentityPayload,
+) -> Result<(), RuntimeError> {
     app_state.set(HostRuntimeState::GuestConnected {
         session_id: session.session_id,
         session_trace_id: session.session_trace_id,
@@ -812,7 +857,6 @@ mod tests {
 
     #[derive(Clone)]
     struct FakeBackend {
-        identity: IdentityPayload,
         reports: Arc<Mutex<VecDeque<Vec<u8>>>>,
         operations: Arc<Mutex<Vec<String>>>,
         cleanup_called: Arc<AtomicBool>,
@@ -821,7 +865,6 @@ mod tests {
     impl FakeBackend {
         fn new(reports: Vec<Vec<u8>>) -> Self {
             Self {
-                identity: test_identity(),
                 reports: Arc::new(Mutex::new(reports.into())),
                 operations: Arc::new(Mutex::new(Vec::new())),
                 cleanup_called: Arc::new(AtomicBool::new(false)),
@@ -842,10 +885,6 @@ mod tests {
     }
 
     impl HostBackend for FakeBackend {
-        fn identity(&self) -> &IdentityPayload {
-            &self.identity
-        }
-
         fn open_input_reader(&self) -> Result<Box<dyn InputReportReader>, HostHidError> {
             Ok(Box::new(FakeInputReader {
                 reports: Arc::clone(&self.reports),
@@ -949,6 +988,7 @@ mod tests {
                     guest_pid: 1234,
                 },
                 &AppState::new(),
+                test_identity(),
                 Arc::new(backend_for_server),
                 &server_stop,
             )
@@ -1034,6 +1074,7 @@ mod tests {
                         guest_pid: 1234,
                     },
                     &AppState::new(),
+                    test_identity(),
                     Arc::new(backend),
                     &server_stop,
                 )
@@ -1081,6 +1122,7 @@ mod tests {
                     guest_pid: 1234,
                 },
                 &AppState::new(),
+                test_identity(),
                 Arc::new(backend),
                 &server_stop,
             );
@@ -1142,6 +1184,7 @@ mod tests {
                     guest_pid: 1234,
                 },
                 &AppState::new(),
+                test_identity(),
                 Arc::new(backend),
                 &server_stop,
                 Duration::from_millis(100),