Re: [PATCH] Input: synaptics - add transparent pass-through mode for TrackPoint

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Self-deadlock and Use-After-Free/Double-Free of the pass-through serio port during parent driver unbind or port unregistration.
- [Critical] Type confusion in synaptics_pt_open() leads to arbitrary memory corruption when toggling transparent mode.
- [High] Bypassed mutex synchronization in synaptics_pt_write() causes concurrent hardware command injection and data races.
--

--- Patch [1]: [PATCH] Input: synaptics - add transparent pass-through mode for TrackPoint ---
commit 1a99c1461a4c3bd59a184b3551c27088c1845e41
Author: laserology oss <[email protected]>

Input: synaptics - add transparent pass-through mode for TrackPoint

This patch reworks an earlier implementation to add a transparent pass-through
mode for Synaptics touchpads with a TrackPoint. It documents the interface and
modifies the protocol handling to support relaying the raw byte stream of the
pass-through guest.

> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 2170bbe4c5897..29e9a0c24dc23 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c

[ ... ]

>  static int synaptics_pt_write(struct serio *serio, u8 c)
>  {
>  	struct psmouse *parent = psmouse_from_serio(serio->parent);
> +	struct synaptics_data *priv = parent->private;
>  	u8 rate_param = SYN_PS_CLIENT_CMD; /* indicates that we want pass-through port */
>  	int error;
>  
> +	if (READ_ONCE(priv->transparent_mode))
> +		return parent->ps2dev.serio->write(parent->ps2dev.serio, c);
> +

[Severity: High]
Does this bypass the parent's ps2_cmd_mutex? Since the child port uses its
own isolated ps2_cmd_mutex, returning parent->ps2dev.serio->write(...)
directly could allow concurrent hardware commands. This could interleave raw
PS/2 bytes if userspace triggers a command on the parent device (like
changing transparent_mode) while the child is writing.

>  	error = ps2_sliced_command(&parent->ps2dev, c);
>  	if (error)
>  		return error;

[ ... ]

> @@ -669,9 +747,30 @@ static void synaptics_pt_stop(struct serio *serio)
>  {
>  	struct psmouse *parent = psmouse_from_serio(serio->parent);
>  	struct synaptics_data *priv = parent->private;
> +	bool exit_mode;
>  
> -	guard(serio_pause_rx)(parent->ps2dev.serio);
> -	priv->pt_port = NULL;
> +	exit_mode = READ_ONCE(priv->transparent_mode);
> +
> +	/*
> +	 * The guest is going away. Take the hardware out of transparent
> +	 * mode first so the device stops relaying the guest's byte stream
> +	 * and resumes producing host packets, then update the software state
> +	 * with RX paused so the interrupt handler never observes a torn
> +	 * configuration.
> +	 */
> +	if (exit_mode) {
> +		if (synaptics_exit_transparent_mode(parent))
> +			psmouse_warn(parent,
> +				     "failed to exit transparent mode after pass-through port removal\n");
> +	}

[Severity: High]
Does this create a data race if a driver like serio_raw is bound to the child
port? Unbinding serio_raw invokes synaptics_pt_stop() without holding the
global psmouse_mutex. Modifying hardware state and priv->transparent_mode
here could race with synaptics_set_transparent_mode() running concurrently
on the parent device.

> +
> +	{
> +		guard(serio_pause_rx)(parent->ps2dev.serio);
> +
> +		priv->pt_port = NULL;
> +		WRITE_ONCE(priv->transparent_mode, false);
> +		synaptics_update_protocol_handler(parent);
> +	}
>  }
>  
>  static int synaptics_pt_open(struct serio *serio)
> @@ -681,6 +780,7 @@ static int synaptics_pt_open(struct serio *serio)
>  
>  	guard(serio_pause_rx)(parent->ps2dev.serio);
>  	priv->pt_port_open = true;
> +	priv->pt_psmouse = psmouse_from_serio(serio);

[Severity: Critical]
Can this cast cause memory corruption? If userspace binds a non-psmouse
driver (like serio_raw) to the pass-through port, the driver data points to a
different structure (e.g., struct serio_raw). This caches a bogus pointer in
priv->pt_psmouse, which is later dereferenced in
synaptics_set_transparent_mode().

>  
>  	return 0;
>  }

[ ... ]

> +	/*
> +	 * The touchpad is stopped for the duration of the mode switch, but the
> +	 * pass-through guest keeps streaming. It may therefore be left with a
> +	 * partially assembled packet from the previous mode. Drop it so the
> +	 * stream restarts cleanly in the new mode and no stale bytes are
> +	 * misparsed as pointer motion or spurious button clicks.
> +	 */
> +	if (READ_ONCE(priv->pt_port)) {
> +		struct psmouse *child = READ_ONCE(priv->pt_psmouse);
> +
> +		if (child && child->state == PSMOUSE_ACTIVATED)
> +			psmouse_set_state(child, PSMOUSE_ACTIVATED);

[Severity: Critical]
If a different driver (like serio_raw) is bound, this could interpret
arbitrary memory as a struct psmouse. By manipulating the serio_raw queue
buffer to match PSMOUSE_ACTIVATED and injecting a pointer, could this result
in psmouse_set_state() dereferencing an attacker-controlled pointer to
acquire a spinlock?

> +	}
> +
> +	return len;
> +}

[ ... ]

>  static void synaptics_disconnect(struct psmouse *psmouse)
>  {
>  	struct synaptics_data *priv = psmouse->private;
> @@ -1448,10 +1649,24 @@ static void synaptics_disconnect(struct psmouse *psmouse)
>  	 */
>  	psmouse_smbus_cleanup(psmouse);
>  
> +	if (READ_ONCE(priv->transparent_mode)) {
> +		synaptics_exit_transparent_mode(psmouse);
> +		WRITE_ONCE(priv->transparent_mode, false);
> +	}
> +
>  	if (!priv->absolute_mode &&
>  			SYN_ID_DISGEST_SUPPORTED(priv->info.identity))
>  		device_remove_file(&psmouse->ps2dev.serio->dev,
>  				   &psmouse_attr_disable_gesture.dattr);
> +	if (SYN_CAP_PASS_THROUGH(priv->info.capabilities))
> +		device_remove_file(&psmouse->ps2dev.serio->dev,
> +				   &psmouse_attr_transparent_mode.dattr);
> +
> +	if (priv->pt_serio) {
> +		serio_unregister_port(priv->pt_serio);
> +		kfree(priv->pt_serio);
> +		priv->pt_serio = NULL;
> +	}

[Severity: Critical]
Can this cause a use-after-free, double-free, and self-deadlock?

If the parent port is unregistered (e.g. physical removal), the serio core
traverses the tree depth-first and synchronously destroys the child
pass-through port, freeing its memory. When synaptics_disconnect() is then
called for the parent, calling serio_unregister_port() and kfree() on the
dangling priv->pt_serio pointer could use freed memory and deadlock on
serio_mutex, which is already held by the core's unregistration path.

Alternatively, if userspace manually unbinds the parent driver via sysfs:

psmouse_disconnect() (acquires psmouse_mutex)
  synaptics_disconnect()
    serio_unregister_port()
      (unbinds child driver)
      psmouse_disconnect() for child
        (tries to acquire psmouse_mutex)

Could this sequence lead to a permanent self-deadlock on psmouse_mutex?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1