Re: [PATCH v2 1/5] phy: cpcap-usb: Prevent line glitches from triggering sysrq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying system-wide SysRq state from a leaf PHY driver is a severe layer violation, and the implementation fails to disable SysRq in many configurations while also overwriting administrative changes.
- [High] Lockless manipulation of the global SysRq state introduces a data race that corrupts the input subsystem's handler list, leading to a kernel panic.
--

commit 1e47bb3741a2d7707b55cf24510e851f11fad240
Author: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>

phy: cpcap-usb: Prevent line glitches from triggering sysrq

This commit temporarily disables sysrq support when switching between
USB and debug serial port modes to prevent line glitches from
triggering spurious sysrq events.

> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> index 7cb020dd3423c..d1c70c7153466 100644
> --- a/drivers/phy/motorola/phy-cpcap-usb.c
> +++ b/drivers/phy/motorola/phy-cpcap-usb.c

[ ... ]

> @@ -413,7 +414,12 @@ static int cpcap_usb_gpio_set_mode(struct cpcap_phy_ddata *ddata,
>  
>  static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
>  {
> -	int error;
> +	int old_sysrq_mask, error;
> +
> +	/* Disable sysrq to prevent random sysrq events on line glitches */
> +	old_sysrq_mask = sysrq_mask();
> +	if (old_sysrq_mask & 1)
> +		sysrq_toggle_support(old_sysrq_mask & ~1);

[Severity: High]
Is it safe to modify the global sysrq state from a leaf PHY driver?
Arbitrarily toggling global SysRq state affects all input devices (keyboard,
network, etc.) system-wide.

Also, checking (old_sysrq_mask & 1) assumes bit 0 is always set when sysrq
is enabled. If the system uses a custom mask without bit 0 (like 0x01b6),
would this fail to disable sysrq and leave the system vulnerable to the
glitch panic?

Additionally, calling sysrq_toggle_support() locklessly from this background
worker path can race with administrative sysctl changes or other drivers.
If this races with another thread calling sysrq_toggle_support(), couldn't
both threads observe that sysrq_on() is false and concurrently call
sysrq_register_handler()?

Since sysrq_register_handler() uses input_register_handler() on a single
statically allocated sysrq_handler, registering it twice would corrupt the
input_handler_list and lead to a crash.

>  	/* Disable lines to prevent glitches from waking up mdm6600 */
>  	error = cpcap_usb_gpio_set_mode(ddata, CPCAP_UNKNOWN_DISABLED);

[ ... ]

>  out_err:
> -	dev_err(ddata->dev, "%s failed with %i\n", __func__, error);
> +	if (old_sysrq_mask & 1)
> +		sysrq_toggle_support(old_sysrq_mask);

[Severity: High]
By unconditionally restoring the old mask here, could this accidentally
clobber concurrent administrative changes made via sysctl kernel.sysrq while
the mode switch was happening?

> +
> +	if (error)
> +		dev_err(ddata->dev, "%s failed with %i\n", __func__, error);
>  
>  	return error;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705075809.1793784-1-ivo.g.dimitrov.75@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy