Re: [PATCH v2 2/2] net: usb: Add Infineon XMM6260 Baseband modem support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver uses bit flags as ad-hoc locks to guard state transitions, leading to race conditions.
- [High] The IPC wake state is updated non-atomically across multiple independent bit flags, exposing corrupted intermediate states.
- [High] The delayed work `modem_work` is not cancelled if `probe()` fails after IRQ registration, leading to a potential use-after-free.
- [High] Failure of `regulator_enable()` is ignored, leading to an unbalanced `regulator_disable()` later.
--

commit df5934ca9fa2794b5f5c3e72cad2823772ca0bd8
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Subject: net: usb: Add Infineon XMM6260 Baseband modem support

This adds support for the Infineon/Intel XMM6260 3G modem, which connects
via the application processor's USB line in HSIC mode. The driver handles
the modem configuration and exposes the USB interface.

> diff --git a/drivers/net/usb/baseband-xmm6260.c b/drivers/net/usb/baseband-xmm6260.c
> new file mode 100644
> index 0000000000000..6710e46f41704
> --- /dev/null
> +++ b/drivers/net/usb/baseband-xmm6260.c

[ ... ]

> +static void set_ipc_ap_wake(struct baseband_xmm_data *priv,
> +			    enum baseband_xmm_ipc_ap_wake_state state)
> +{
> +	for (int i = 0; i < BASEBAND_XMM_IPC_AP_WAKE_MAX; i++)
> +		if (state & BIT(i))
> +			set_bit(i, priv->state);
> +		else
> +			clear_bit(i, priv->state);
> +}

[Severity: High]
Is it possible for the IRQ handler to read an invalid intermediate state here?
Since the bits are updated sequentially without a lock, transitioning from
state 3 to 0 could momentarily expose state 2. This could potentially cause
the threaded IRQ handler using get_ipc_ap_wake() to execute the wrong branch
of its state machine.

[ ... ]

> +static void baseband_xmm_reset(struct baseband_xmm_data *priv)
> +{
> +	int ret;
> +
> +	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +
> +	if (!test_bit(BASEBAND_XMM_STATE_POWERED, priv->state)) {
> +		ret = regulator_enable(priv->vbat_supply);
> +		if (ret)
> +			dev_err(priv->dev,
> +				"failed to enable vbat power supply\n");
> +
> +		set_bit(BASEBAND_XMM_STATE_POWERED, priv->state);
> +	}

[Severity: High]
If regulator_enable() fails, the driver proceeds and unconditionally sets the
BASEBAND_XMM_STATE_POWERED bit. Does this lead to an unbalanced
regulator_disable() later in baseband_xmm_poweroff()? Disabling a regulator
that was never successfully enabled could trigger a kernel warning and turn
off the regulator for other shared consumers.

[ ... ]

> +static void baseband_xmm_poweroff(struct baseband_xmm_data *priv)
> +{
> +	/*
> +	 * The test_bit check prevents poweroff from being recursively
> +	 * called during USB device deregistration. USB device
> +	 * deregistration can be triggered by the driver by calling this
> +	 * function or by some external event. The first case will cause
> +	 * a recursive call by the notifier if not handled, while the
> +	 * second case requires this call to handle the USB controller
> +	 * properly.
> +	 */
> +	if (test_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state))
> +		return;
> +
> +	set_bit(BASEBAND_XMM_STATE_PROTECTED, priv->state);
> +	set_bit(BASEBAND_XMM_STATE_POWEROFF, priv->state);

[Severity: High]
Can concurrent calls bypass these checks and cause a double teardown? Because
test_bit() followed by set_bit() is not an atomic operation, concurrent calls
from the workqueue, USB notifier, and rfkill callbacks could simultaneously
observe BASEBAND_XMM_STATE_POWEROFF as false. This could result in duplicate
pwrseq_power_off() and regulator_disable() calls. Should a proper mutex be
used instead of bit flags for synchronization?

[ ... ]

> +static int baseband_xmm_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	ret = devm_request_threaded_irq(dev, priv->irq, NULL,
> +					&baseband_hostwake_interrupt,
> +					IRQF_ONESHOT | irqflags,
> +					"modem-hostwake", priv);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to register IRQ %d\n", priv->irq);
> +
> +	priv->rfkill_dev = rfkill_alloc("xmm-modem", dev, RFKILL_TYPE_WWAN,
> +					&baseband_xmm_rfkill_ops, priv);
> +	if (!priv->rfkill_dev)
> +		return -ENOMEM;
> +
> +	ret = rfkill_register(priv->rfkill_dev);
> +	if (ret) {
> +		rfkill_destroy(priv->rfkill_dev);
> +		return dev_err_probe(dev, ret,
> +				     "failed to register WWAN rfkill\n");
> +	}

[Severity: High]
If probe() fails at rfkill_register() or later, is there a risk of a
use-after-free with the delayed work? The IRQ is registered and enabled
beforehand, which could cause the interrupt handler to schedule modem_work.
Since the devres framework automatically frees the priv structure and tears
down the IRQ but does not cancel the scheduled delayed work, the work might
execute later and dereference the freed priv pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523084408.50346-1-clamor95@gmail.com?part=2