Re: [PATCH] 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:
- [Critical] Missing pass-through port check in psmouse_receive_byte causes type confusion and control flow hijacking for non-psmouse parent ports.
- [High] Blind cast of pass-through port's drvdata in synaptics_set_transparent_mode leads to type confusion and NULL pointer dereferences.
- [Medium] Hardware and software states become permanently desynchronized if the guest port is stopped while transparent mode is active.
- [Medium] Unprotected updates to protocol_handler and pktsize in synaptics_set_transparent_mode() cause a data race with the RX interrupt.
- [Medium] Skipping synaptics_set_mode() during reconnect in transparent mode leaves the hardware uninitialized when transparent mode is later disabled.

Pre-existing issues:
- [High] Failing to create sysfs attributes leaks the pass-through port and causes a use-after-free on its access.
--

--- Patch [1]: [PATCH] Input: synaptics - add transparent pass-through mode for TrackPoint ---
commit 1fc824e1e4a325dcff29e0fb66487d7ede60f009
Author: Laserology OSS <[email protected]>
Subject: Input: synaptics - add transparent pass-through mode for TrackPoint

This patch adds a transparent pass-through mode for Synaptics touchpads
with a pass-through port. It allows the guest device to run at its full
poll rate while temporarily disabling the touchpad.

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

> diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
[ ... ]
> @@ -392,8 +392,18 @@ static void psmouse_receive_byte(struct ps2dev *ps2dev, u8 data)
>  		}
>  
>  		if (psmouse->packet[1] == PSMOUSE_RET_ID) {
> +			struct serio *serio = ps2dev->serio;
> +			struct psmouse *parent_psmouse = NULL;
> +
>  			__psmouse_set_state(psmouse, PSMOUSE_IGNORE);
> -			serio_reconnect(ps2dev->serio);
> +			/* Some devices need parent to be reconnected instead */
> +			if (serio->parent)
> +				parent_psmouse = psmouse_from_serio(serio->parent);
> +			if (parent_psmouse && parent_psmouse->pt_reconnect_parent &&
> +			    parent_psmouse->pt_reconnect_parent(parent_psmouse))

[Severity: Critical]
Is it safe to unconditionally cast serio->parent's driver data to a psmouse
struct here?

If the parent port is a multiplexer driven by ps2mult, its driver data
would be a different struct. Accessing parent_psmouse->pt_reconnect_parent
could then lead to type confusion and read a bogus function pointer,
potentially causing a crash or arbitrary execution when called.

> +				serio_reconnect(serio->parent);
> +			else
> +				serio_reconnect(serio);
>  			return;
>  		}
>  
> 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
[ ... ]
> @@ -672,6 +750,8 @@ static void synaptics_pt_stop(struct serio *serio)
>  
>  	guard(serio_pause_rx)(parent->ps2dev.serio);
>  	priv->pt_port = NULL;
> +
> +	synaptics_update_protocol_handler(parent);

[Severity: Medium]
If the guest port is unbound while transparent mode is active, priv->pt_port
becomes NULL and synaptics_update_protocol_handler() will revert the software
handler to synaptics_process_byte. Will this cause the hardware and software
states to become permanently desynchronized?

The hardware would still be in transparent mode sending 3-byte packets, but
the software would expect 6-byte absolute packets, which could lead to
erratic pointer behavior.

>  }
[ ... ]
> @@ -1438,6 +1535,80 @@ PSMOUSE_DEFINE_ATTR(disable_gesture, S_IWUSR | S_IRUGO, NULL,
[ ... ]
> +	WRITE_ONCE(priv->transparent_mode, value);
> +
> +	synaptics_update_protocol_handler(psmouse);

[Severity: Medium]
Should this call to synaptics_update_protocol_handler() be protected by
guard(serio_pause_rx)? It updates psmouse->protocol_handler and
psmouse->pktsize outside of the lock.

If an interrupt arrives concurrently, psmouse_receive_byte() might read the
new pktsize but the old protocol_handler, leading to incorrect packet
processing.

> +
> +	/*
> +	 * 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 = psmouse_from_serio(priv->pt_port);
> +
> +		if (child && child->state == PSMOUSE_ACTIVATED)
> +			psmouse_set_state(child, PSMOUSE_ACTIVATED);
> +	}

[Severity: High]
Does this cast safely handle the case where the guest port is unbound or
bound to a different driver?

If the port is unbound, the drvdata is NULL and psmouse_from_serio()
evaluates to a negative offset, causing a panic when child->state is checked.

If it is bound to another driver like serio_raw, this causes type confusion
and psmouse_set_state() could dereference an invalid pointer, leading to
memory corruption.

> +
> +	return len;
> +}
[ ... ]
> @@ -1478,8 +1655,16 @@ 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)) {
> +			error = synaptics_enter_transparent_mode(psmouse);
> +			if (!error) {
> +				serio_reconnect(priv->pt_port);
> +				return 0;
> +			}

[Severity: Medium]
Returning early here bypasses the normal synaptics_set_mode() initialization
during reconnect. If the user later disables transparent mode via sysfs,
synaptics_exit_transparent_mode() only restores the previous mode byte but
won't re-enable Advanced Gesture Mode or other configurations set up by
synaptics_set_mode().

Could this leave the touchpad uninitialized after a suspend/resume cycle?

> +		} else {
> +			ps2_command(&psmouse->ps2dev, param, PSMOUSE_CMD_GETID);
> +			error = synaptics_detect(psmouse, 0);
> +		}
>  	} while (error && ++retry < 3);
[ ... ]
> @@ -1691,9 +1869,26 @@ static int synaptics_init_ps2(struct psmouse *psmouse,
[ ... ]
>   init_fail:
> +	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);
>  	kfree(priv);
>  	return err;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but if device_create_file() fails
in synaptics_init_ps2(), the code jumps to init_fail, frees the priv structure,
and returns an error without unregistering the pass-through serio child port.

The child port remains active, and any subsequent interaction with it could
trigger callbacks that dereference the freed parent->private pointer,
resulting in a use-after-free. Should the port be unregistered during
cleanup?

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