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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The global SysRq support mask is permanently altered if an error occurs during UART mode configuration.
- [High] Flawed bitwise logic fails to effectively disable SysRq in system configurations that use specific feature masks, leaving the system vulnerable to glitches.
- [High] The driver modifies global system configuration to mask a local hardware glitch, introducing system-wide side effects and race conditions.

Pre-existing issues:
- [High] A Use-After-Free vulnerability exists because devres-managed IRQs can queue delayed work on freed memory after `remove()` completes.
- [High] Hardware teardown in `remove()` races with the background worker, leading to undefined hardware state.
--

commit d89e10225f70bc2771f7d996decb42920d32fe88
Author: Tony Lindgren <tony@atomide.com>

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

This commit attempts to prevent random sysrq events when switching from USB
to debug serial port mode. It temporarily disables sysrq during mode changes
to mask line glitches.

> diff --git a/drivers/phy/motorola/phy-cpcap-usb.c b/drivers/phy/motorola/phy-cpcap-usb.c
> --- 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 sysrq, error;
> +
> +	/* Disable sysrq to prevent random sysrq events on line glitches */
> +	sysrq = sysrq_mask();
> +	if (sysrq & 1)
> +		sysrq_toggle_support(sysrq & ~1);

[Severity: High]
Does modifying the global sysrq state here create a race condition?

If an administrator concurrently modifies the sysrq settings via
/proc/sys/kernel/sysrq, will the restoration call at the end of this
function blindly overwrite their new state?

[Severity: High]
Will this correctly disable sysrq if the system is configured with a
specific feature mask?

If the administrator clears the SYSRQ_ENABLE_ALL bit (bit 0) but leaves
other features enabled, sysrq & 1 evaluates to 0, bypassing the disablement.

If bit 0 and other bits are set, sysrq & ~1 disables the 'enable all' flag
but leaves the specific logging/dumping features actively enabled.

[ ... ]

> @@ -448,6 +454,9 @@ static int cpcap_usb_set_uart_mode(struct cpcap_phy_ddata *ddata)
>  	if (error)
>  		goto out_err;
>  
> +	if (sysrq & 1)
> +		sysrq_toggle_support(sysrq);
> +
>  	return 0;
>  
>  out_err:

[Severity: High]
Does this error path permanently alter the sysrq support mask?

If a failure occurs during the GPIO or pinctrl mode configuration, the
function jumps to out_err and bypasses the restoration of the sysrq state.
Could this permanently disable sysrq features system-wide?


[Severity: High]
This is a pre-existing issue, but is there a use-after-free vulnerability
during device teardown?

Looking at cpcap_phy_irq_thread():

drivers/phy/motorola/phy-cpcap-usb.c:cpcap_phy_irq_thread() {
    if (!atomic_read(&ddata->active))
        return IRQ_NONE;
    
    schedule_delayed_work(&ddata->detect_work, msecs_to_jiffies(1));
}

And in cpcap_usb_phy_remove(), cancel_delayed_work_sync() is called, but
because devm_request_threaded_irq() is used, the IRQ is freed after
remove() finishes.

If the IRQ thread is preempted immediately after reading ddata->active,
can it resume during free_irq(), and queue detect_work on freed memory
after devres frees ddata?


[Severity: High]
This is a pre-existing issue, but does the hardware teardown in
cpcap_usb_phy_remove() race with the background worker?

Looking at cpcap_usb_phy_remove():

drivers/phy/motorola/phy-cpcap-usb.c:cpcap_usb_phy_remove() {
    cpcap_usb_set_idle_mode(ddata);
    cpcap_usb_try_musb_mailbox(ddata, MUSB_VBUS_OFF);
    
    usb_remove_phy(&ddata->phy);
    cancel_delayed_work_sync(&ddata->detect_work);
}

The driver attempts to force an idle state before stopping the background
worker cpcap_usb_detect().

Since the worker thread doesn't check ddata->active, could it concurrently
modify the same regmap registers and pinctrl states, leaving the hardware
in an undefined state?

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