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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Type confusion leading to out-of-bounds read/write due to relying solely on hardware port ID for driver validation.
- [High] Unsynchronized hardware commands interleave with pass-through byte stream, corrupting PS/2 communication.
- [High] Child port is leaked when parent driver probe fails, leading to a Use-After-Free crash.
- [Medium] Escape sequence aborts if the first byte is not ACKed, permanently locking the hardware in transparent mode.
--

--- Patch [1]: [PATCH v5] Input: synaptics - add pass-through mode for TrackPoint ---
commit 0baba0d9c35febab280e4dc02e618df2555881dc
Author: Evan Lawrence <[email protected]>
Subject: Input: synaptics - add pass-through mode for TrackPoint

This commit introduces a transparent pass-through mode for Synaptics touchpads
with a PS/2 guest port, allowing the guest (e.g., TrackPoint) to bypass the
touchpad's low poll rate by relaying raw byte streams directly. It adds a sysfs
attribute to toggle the mode at runtime.

Link: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -636,12 +650,90 @@ static void synaptics_set_rate(struct psmouse *psmouse, unsigned int rate)
>  /*****************************************************************************
>   *	Synaptics pass-through PS/2 port support
>   ****************************************************************************/

[ ... ]

> +static int synaptics_exit_transparent_mode(struct psmouse *psmouse)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +	int error;
> +
> +	/* Send scaling 2:1, 1:1 to exit transparent mode */
> +	error = ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE21);
> +	if (error)
> +		return error;
> +	error = ps2_command(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11);
> +	if (error)
> +		return error;

[Severity: Medium]
Will a failure on the first byte permanently lock the hardware in transparent
mode?

If the guest device is missing, resetting, or unresponsive, the first
PSMOUSE_CMD_SETSCALE21 byte may time out waiting for an ACK. When ps2_command()
returns an error, this function aborts without sending the crucial second byte
(PSMOUSE_CMD_SETSCALE11).

Since synaptics_pt_stop() clears the software transparent_mode flag even if the
hardware exit fails, doesn't this leave the driver fully desynchronized from
the hardware state, leaving the touchpad permanently unresponsive until a
reboot?

> +
> +	/* Re-enter the regular operating mode of the touchpad */
> +	error = synaptics_mode_cmd(psmouse, priv->mode & ~SYN_BIT_TRANSPARENT_MODE);
> +	if (error)
> +		return error;
> +
> +	priv->mode &= ~SYN_BIT_TRANSPARENT_MODE;
> +
> +	return 0;
> +}
> +
>  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;
>  
> +	guard(mutex)(&priv->pt_mutex);
> +
> +	if (READ_ONCE(priv->transparent_mode))
> +		return parent->ps2dev.serio->write(parent->ps2dev.serio, c);

[Severity: High]
Can this lead to corrupted PS/2 communication due to interleaved byte streams?

When transparent_mode is active, synaptics_pt_write() bypasses ps2_cmd_mutex to
forward raw guest bytes. However, other paths like synaptics_set_disable_gesture()
or synaptics_reconnect() can send PS/2 commands while holding ps2_cmd_mutex but
without checking transparent_mode or holding pt_mutex.

For example, if a user writes to the disable_gesture sysfs attribute while the
pass-through guest is active, won't the host command bytes and guest data
bytes interleave on the physical PS/2 bus?

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

[ ... ]

> @@ -681,6 +795,19 @@ static int synaptics_pt_open(struct serio *serio)
>  	guard(serio_pause_rx)(parent->ps2dev.serio);
>  	priv->pt_port_open = true;
>  
> +	/*
> +	 * In transparent mode every byte on the parent serio belongs to
> +	 * the pass-through guest and is not a BAT announcement.  Skip
> +	 * BAT detection on the child as well so that guest motion data
> +	 * that happens to look like 0xAA 0x00 does not trigger a
> +	 * spurious parent reconnect.
> +	 */
> +	if (READ_ONCE(priv->transparent_mode) &&
> +	    serio->id.type == SERIO_PS_PSTHRU) {
> +		struct psmouse *child = psmouse_from_serio(serio);
> +		WRITE_ONCE(child->pt_bypass_bat, true);
> +	}

[Severity: High]
Can this cause an out-of-bounds write if the child port is bound to a
different driver?

Checking serio->id.type == SERIO_PS_PSTHRU only verifies the hardware port
type, but does not guarantee the bound driver is psmouse. If a privileged user
unbinds the psmouse driver and binds serio_raw instead, won't psmouse_from_serio()
cast a smaller private data structure to struct psmouse, causing
WRITE_ONCE(child->pt_bypass_bat, true) to corrupt slab memory?

This type confusion also appears to happen in synaptics_pt_close(),
synaptics_pass_pt_packet(), synaptics_pt_activate(), and
synaptics_set_transparent_mode().

> +
>  	return 0;
>  }

[ ... ]

> @@ -1447,10 +1692,20 @@ static void synaptics_disconnect(struct psmouse *psmouse)
>  	 */
>  	psmouse_smbus_cleanup(psmouse);
>  
> +	if (READ_ONCE(priv->transparent_mode)) {
> +		guard(mutex)(&priv->pt_mutex);
> +		synaptics_exit_transparent_mode(psmouse);
> +		WRITE_ONCE(priv->transparent_mode, false);
> +		WRITE_ONCE(psmouse->pt_bypass_bat, 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);
>  
>  	synaptics_reset(psmouse);
>  	kfree(priv);

[Severity: High]
Does this leak the pass-through child port if probe fails, leading to a
use-after-free?

If psmouse_connect() fails late in probe (for example, during
input_register_device() due to memory exhaustion), it calls the driver's
disconnect callback, which frees the priv structure here.

Since there is no call to serio_unregister_port() for the child port, it
remains registered. When the system asynchronously finishes adding the leaked
port, any driver binding to it will call synaptics_pt_start(). Won't this
extract and dereference the freed parent driver data, causing a crash?

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