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

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] TOCTOU races in synaptics_pt_stop and synaptics_set_transparent_mode cause hardware and software state desynchronization.
- [High] synaptics_pt_write bypasses ps2_cmd_mutex in transparent mode, corrupting the PS/2 bus byte stream.
- [High] synaptics_reconnect lacks pt_mutex synchronization, causing races with concurrent guest writes.
- [High] Delayed NULL pointer dereference (or Use-After-Free) in synaptics_reconnect due to a TOCTOU read of priv->pt_port.

Pre-existing issues:
- [High] Ineffective driver type check allows type confusion and out-of-bounds access in pass-through port handling.
- [High] Incomplete fix for the child port leak: child port leaks if input_register_device fails, leading to UAF.
--

--- Patch [1]: [PATCH v4] Input: synaptics - add transparent pass-through mode for TrackPoint ---
commit 7d4fbfed011585417421459058126e46e98ff9df
Author: Evan Lawrence <[email protected]>

Input: synaptics - add transparent pass-through mode for TrackPoint
    
This commit introduces a transparent pass-through mode for Synaptics touchpads,
allowing a pass-through guest to operate at a higher poll rate. It adds a sysfs
attribute to toggle the mode, handles hardware state switches, and adjusts the
protocol handler dynamically.

> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index 2170bbe4c5897..2a004ae34158d 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;
>  
> +	guard(mutex)(&priv->pt_mutex);
> +
> +	if (READ_ONCE(priv->transparent_mode))
> +		return parent->ps2dev.serio->write(parent->ps2dev.serio, c);

[Severity: High]
Does bypassing ps2_cmd_mutex here allow raw guest bytes to interleave with host
commands on the PS/2 bus?

In drivers/input/mouse/synaptics.c:synaptics_pt_write():
When transparent mode is active, this fast-path directly writes to the
hardware under pt_mutex, but does not acquire ps2_cmd_mutex. If a sysfs
attribute on the parent touchpad is updated concurrently, it issues a
ps2_command(), potentially interleaving guest bytes and host commands and
corrupting communication with the touchpad.

>  	error = ps2_sliced_command(&parent->ps2dev, c);
[ ... ]
> @@ -669,9 +749,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 = READ_ONCE(priv->transparent_mode);
>  
> -	guard(serio_pause_rx)(parent->ps2dev.serio);
> -	priv->pt_port = NULL;
> +	/*
> +	 * The guest is going away. Hold the pass-through mutex across
> +	 * the hardware exit and the software state update so a
> +	 * concurrent synaptics_pt_write() cannot observe the old
> +	 * transparent_mode after the hardware has already been
> +	 * switched back to normal operation.
> +	 */
> +	guard(mutex)(&priv->pt_mutex);
> +
> +	if (exit_mode) {
> +		if (synaptics_exit_transparent_mode(parent))

[Severity: High]
Does reading exit_mode outside of pt_mutex allow the hardware and software
state to become desynchronized?

In drivers/input/mouse/synaptics.c:synaptics_pt_stop():
If synaptics_pt_stop() is called concurrently with 
synaptics_set_transparent_mode(), exit_mode is cached before acquiring the lock.
The mode switch could enable hardware transparent mode, and then
synaptics_pt_stop() acquires the lock, skips the hardware exit because the
cached exit_mode was false, but forces the software state to normal mode.

> +			psmouse_warn(parent,
> +				     "failed to exit transparent mode after pass-through port removal\n");
> +	}
[ ... ]
> @@ -709,7 +822,7 @@ static void synaptics_pass_pt_packet(struct synaptics_data *priv, u8 *packet)
>  
>  	serio_interrupt(ptport, packet[1], 0);
>  
> -	if (priv->pt_port_open) {
> +	if (priv->pt_port_open && ptport->id.type == SERIO_PS_PSTHRU) {
>  		struct psmouse *child = psmouse_from_serio(ptport);
>  
>  		if (child->state == PSMOUSE_ACTIVATED) {

[Severity: High]
This is a pre-existing issue, but does checking the hardware ID reliably
verify the bound software driver type?

In drivers/input/mouse/synaptics.c:synaptics_pass_pt_packet():
If a root user unbinds the psmouse driver from the pass-through child port
and binds a different driver like serio_raw, this check passes because it
evaluates the hardware ID. The foreign driver's private data is then
downcasted to struct psmouse, potentially allowing out-of-bounds writes to
child->state or child->oob_data_type.

> @@ -724,7 +837,14 @@ static void synaptics_pass_pt_packet(struct synaptics_data *priv, u8 *packet)
[ ... ]
> +static ssize_t synaptics_set_transparent_mode(struct psmouse *psmouse,
> +					      void *data, const char *buf,
> +					      size_t len)
> +{
> +	struct synaptics_data *priv = psmouse->private;
> +	unsigned int value;
> +	int err;
> +
> +	err = kstrtouint(buf, 10, &value);
> +	if (err)
> +		return err;
> +
> +	if (value > 1)
> +		return -EINVAL;
> +
> +	if (value == READ_ONCE(priv->transparent_mode))
> +		return len;
> +
> +	/* Transparent mode only makes sense while the guest is attached */
> +	if (value && !READ_ONCE(priv->pt_port))
> +		return -ENODEV;
> +
> +	/*
> +	 * Switch the hardware first and only commit the new mode and protocol
> +	 * handler once it succeeded, so a failing command cannot leave the
> +	 * driver with bookkeeping that does not match the device. The touchpad
> +	 * is deactivated for the duration of this write (see
> +	 * psmouse_attr_set_helper), so no live data can be misparsed in
> +	 * between.  Hold the pass-through mutex so that a concurrent
> +	 * synaptics_pt_write() cannot inject a byte onto the bus in the
> +	 * middle of the mode-switch command sequence.
> +	 */
> +	guard(mutex)(&priv->pt_mutex);

[Severity: High]
Does checking pt_port outside of pt_mutex create a race condition with
synaptics_pt_stop()?

In drivers/input/mouse/synaptics.c:synaptics_set_transparent_mode():
Since the pt_port check happens before acquiring pt_mutex,
synaptics_pt_stop() could concurrently destroy pt_port before the mode switch
executes, leading to misconfigured protocol handlers.

> +
> +	if (value) {
> +		err = synaptics_enter_transparent_mode(psmouse);
[ ... ]
> @@ -1478,8 +1681,35 @@ static int synaptics_reconnect(struct psmouse *psmouse)
>  			 */
>  			ssleep(1);
>  		}
> -		ps2_command(&psmouse->ps2dev, param, PSMOUSE_CMD_GETID);
> -		error = synaptics_detect(psmouse, 0);
> +		if (READ_ONCE(priv->transparent_mode) && READ_ONCE(priv->pt_port)) {
> +			/*
> +			 * psmouse_reset() above put the touchpad back into its
> +			 * power-on state, so run the full initialization before
> +			 * re-entering transparent mode. Otherwise the device
> +			 * would come back missing the configuration set up by
> +			 * synaptics_set_mode() (e.g. Advanced Gesture Mode) and
> +			 * would stay uninitialized once transparent mode is
> +			 * disabled again.
> +			 */
> +			error = synaptics_query_hardware(psmouse, &info);
> +			if (!error &&
> +			    (info.identity != priv->info.identity ||
> +			     info.model_id != priv->info.model_id ||
> +			     info.capabilities != priv->info.capabilities ||
> +			     info.ext_cap != priv->info.ext_cap))
> +				error = -ENXIO;
> +			if (!error)
> +				error = synaptics_set_mode(psmouse);
> +			if (!error)
> +				error = synaptics_enter_transparent_mode(psmouse);

[Severity: High]
Can concurrent guest writes interleave with the hardware initialization
sequence here?

In drivers/input/mouse/synaptics.c:synaptics_reconnect():
The entire re-initialization sequence runs without holding pt_mutex. If the
child port driver writes data concurrently, synaptics_pt_write() will see
transparent_mode as true, lock pt_mutex, and inject bytes directly onto the
bus between the PS/2 commands issued here.

> +			if (!error) {
> +				serio_reconnect(priv->pt_port);

[Severity: High]
Can priv->pt_port be NULL or point to freed memory by the time it is accessed
here?

In drivers/input/mouse/synaptics.c:synaptics_reconnect():
priv->pt_port is checked before the lengthy hardware query sequence. If the
child port is concurrently unbound during this time, synaptics_pt_stop() could
nullify and free the port structure, causing a NULL pointer dereference or
use-after-free when passed to serio_reconnect().

> +				return 0;
> +			}
[ ... ]
> @@ -1691,9 +1913,35 @@ static int synaptics_init_ps2(struct psmouse *psmouse,
[ ... ]
> +	/*
> +	 * Register the pass-through port only once all initialization steps
> +	 * that can fail have succeeded, so that a failure in one of the steps
> +	 * above cannot leak a registered child port whose callbacks would
> +	 * dereference the freed synaptics_data.
> +	 */
> +	if (SYN_CAP_PASS_THROUGH(info->capabilities))
> +		synaptics_pt_create(psmouse);
> +
>  	return 0;

[Severity: High]
This is a pre-existing issue, but does this still leak the child port if
the parent's input_register_device() fails later?

In drivers/input/mouse/synaptics.c:synaptics_init_ps2():
Even though synaptics_pt_create() is moved to the end of this function, if
the caller (psmouse_connect()) subsequently fails in input_register_device(),
it aborts and frees the psmouse instance via synaptics_disconnect(). The child
port is never unregistered, and if it is bound later, its handlers will
dereference the freed parent private data.

>  
>   init_fail:

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