[PATCH v3] usb: typec: tcpci: filter masked alerts and quiesce the IRQ

[PATCH v3] usb: typec: tcpci: filter masked alerts and quiesce the IRQ

[Yaohan Li]

tcpci_irq() reads TCPC_ALERT and uses the result for the event
callbacks, while only the IRQ return value is masked with
tcpci->alert_mask.  A masked alert is still latched in TCPC_ALERT even
though it does not assert Alert# (TCPCI spec 4.4.3), so the handler can
run TCPM callbacks for events that the port has masked.  This also
defeats the ALERT_MASK = 0 write during teardown when the write fails.

In addition, tcpci_remove() unregisters the TCPM port, which destroys
its kthread worker, while the devm-managed threaded IRQ can still be
running and queue work on that worker.  tcpci_unregister_port() must
not run before in-flight handlers have finished.

There is a second teardown race through tcpm_tcpc_reset(): the IRQ
handler can call tcpm_init(), which calls tcpci_init(), and that
function publishes the normal alert mask.  If it interleaves with the
teardown clearing of the mask, it can restore the mask after
synchronize_irq() returns and let a later shared IRQ reach the TCPM
port after its worker was destroyed.

Filter the alert status with tcpci->alert_mask before processing it,
and again after the final re-read.  Add an alert_mask_lock and an
alerts_quiesced state: tcpci_quiesce_alerts() sets both atomically, and
tcpci_init() only publishes the mask while alerts are not quiesced.
Return IRQ_NONE immediately when the software mask is zero, so a
quiesced port does not perform an I2C read at all.  In tcpci_remove(),
quiesce the alerts, disable the hardware mask, then call
synchronize_irq() before unregistering the port.  Apply the same
ordering on the probe error path when the final ALERT_MASK enable write
fails.  A handler that starts after synchronize_irq(), including one
triggered on a shared line, sees the zero software mask and returns
IRQ_NONE without touching the TCPM port.

The masked-status handling and the unquiesced teardown both date back to
the original TCPCI driver.  The TCPM port worker was later migrated to
kthread_work, which is the path that reports the use-after-free, but the
missing status filter and IRQ synchronization are the underlying
defects.

The probe ordering that requests the IRQ before the final ALERT_MASK
enable write was added later, so the probe error path needs the same
quiesce sequence as removal.

Tested on 7.4-rc1 with a simulated I2C TCPCI device and KASAN:

- Unpatched: a handler parked in tcpm_cc_change()/kthread_queue_work()
  while the device is unbound produces a slab-use-after-free on the
  destroyed TCPM worker.
- Patched: synchronize_irq() waits for the parked handler; the handler
  queues on the still-live worker, then the worker is destroyed; no
  report.
- Normal CC event: the callback still runs and removal completes without
  a report.
- Shared IRQ raised after synchronize_irq(), with a stale CC alert:
  tcpci_irq() filters it with alert_mask = 0 and returns IRQ_NONE
  without calling tcpm_cc_change().
- Hardware ALERT_MASK = 0 write forced to fail during removal, with a
  continuous alert stream: no TCPM callback after the worker is
  destroyed.
- Final ALERT_MASK enable write forced to fail during probe: the error
  path quiesces the IRQ before unregistering the port.
- Reset-race: the IRQ handler is parked in tcpci_init() while remove
  quiesces and waits; after release, the pre-guard code restored the
  mask and a shared IRQ reached tcpm_cc_change() with a destroyed
  worker (KASAN null-pointer dereference).  The guarded code keeps
  alert_mask at zero, returns IRQ_NONE, and completes without a report.
- Masked/unmasked mix: with TX_SUCCESS masked and CC unmasked in the
  same ALERT, only tcpm_cc_change() runs; the masked TX bit is not
  cleared and remains latched.
- Normal TX_SUCCESS and RX_STATUS alerts still reach
  tcpm_pd_transmit_complete() and tcpm_pd_receive(), respectively.

Real TCPCI hardware and PD traffic remain untested.

Fixes: 74e656d6b055 ("staging: typec: Type-C Port Controller Interface driver (tcpci)")
Fixes: 862a9c0f6848 ("usb: typec: tcpci: fix NULL pointer issue on shared irq case")
Signed-off-by: Yaohan Li <shunsheng1218@gmail.com>
---
Changes in v3:
- Filter status with alert_mask before processing it and after the
  final re-read; drop the v2 chip->stopping flag.
- Add alert_mask_lock/alerts_quiesced so tcpci_init() cannot restore
  the software mask after teardown started, and return IRQ_NONE early
  when the software mask is zero.
- Publish the quiesced state before the hardware mask write, then
  synchronize_irq() before unregistering the port; same ordering on the
  probe error path.
- Point Fixes at the original TCPCI driver and the later probe ordering
  change instead of the file move.
- Re-test remove/inject, shared-IRQ, mask-failure, probe-failure, the
  tcpci_init() reset race, masked/unmasked mixed alerts, and normal
  TX/RX events.  The final metadata-only amend does not change the
  tested driver source.

AI tools assisted with source review, patch iteration and test harness
development. The author reviewed the resulting code and test results.

v2: https://lore.kernel.org/r/20261106091725.1334097-1-shunsheng1218@gmail.com/
v1: https://lore.kernel.org/r/179378589227.74015.8259496684787732118@gmail.com/

 drivers/usb/typec/tcpm/tcpci.c | 44 ++++++++++++++++++++++++++++++----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
index 6717ac914c6a6..1b98940fdaed9 100644
--- a/drivers/usb/typec/tcpm/tcpci.c
+++ b/drivers/usb/typec/tcpm/tcpci.c
@@ -39,6 +39,8 @@ struct tcpci {
 	struct regmap *regmap;
 	unsigned int alert_mask;
 	unsigned int rx_type_mask;
+	spinlock_t alert_mask_lock; /* protects alert_mask and alerts_quiesced */
+	bool alerts_quiesced;
 
 	bool controls_vbus;
 
@@ -648,10 +650,21 @@ static bool tcpci_attempt_vconn_swap_discovery(struct tcpc_dev *tcpc)
 	return false;
 }
 
+static void tcpci_quiesce_alerts(struct tcpci *tcpci)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&tcpci->alert_mask_lock, flags);
+	tcpci->alerts_quiesced = true;
+	WRITE_ONCE(tcpci->alert_mask, 0);
+	spin_unlock_irqrestore(&tcpci->alert_mask_lock, flags);
+}
+
 static int tcpci_init(struct tcpc_dev *tcpc)
 {
 	struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
 	unsigned long timeout = jiffies + msecs_to_jiffies(2000); /* XXX */
+	unsigned long flags;
 	unsigned int reg;
 	int ret;
 
@@ -710,7 +723,10 @@ static int tcpci_init(struct tcpc_dev *tcpc)
 			return ret;
 	}
 
-	tcpci->alert_mask = reg;
+	spin_lock_irqsave(&tcpci->alert_mask_lock, flags);
+	if (!tcpci->alerts_quiesced)
+		WRITE_ONCE(tcpci->alert_mask, reg);
+	spin_unlock_irqrestore(&tcpci->alert_mask_lock, flags);
 
 	return 0;
 }
@@ -722,8 +738,12 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 	int irq_ret;
 	unsigned int raw;
 
+	if (!READ_ONCE(tcpci->alert_mask))
+		return IRQ_NONE;
+
 	tcpci_read16(tcpci, TCPC_ALERT, &status);
-	irq_ret = status & tcpci->alert_mask;
+	status &= READ_ONCE(tcpci->alert_mask);
+	irq_ret = status;
 
 process_status:
 	/*
@@ -806,8 +826,9 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
 		tcpm_pd_transmit_complete(tcpci->port, TCPC_TX_FAILED);
 
 	tcpci_read16(tcpci, TCPC_ALERT, &status);
+	status &= READ_ONCE(tcpci->alert_mask);
 
-	if (status & tcpci->alert_mask)
+	if (status)
 		goto process_status;
 
 	return IRQ_RETVAL(irq_ret);
@@ -854,6 +875,7 @@ struct tcpci *tcpci_register_port(struct device *dev, struct tcpci_data *data)
 	tcpci->dev = dev;
 	tcpci->data = data;
 	tcpci->regmap = data->regmap;
+	spin_lock_init(&tcpci->alert_mask_lock);
 
 	tcpci->tcpc.init = tcpci_init;
 	tcpci->tcpc.get_vbus = tcpci_get_vbus;
@@ -972,12 +994,15 @@ static int tcpci_probe(struct i2c_client *client)
 	/* Enable chip interrupts at last */
 	err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, chip->tcpci->alert_mask);
 	if (err < 0)
-		goto unregister_port;
+		goto quiesce_irq;
 
 	device_set_wakeup_capable(chip->tcpci->dev, true);
 
 	return 0;
 
+quiesce_irq:
+	tcpci_quiesce_alerts(chip->tcpci);
+	synchronize_irq(client->irq);
 unregister_port:
 	tcpci_unregister_port(chip->tcpci);
 	return err;
@@ -988,10 +1013,19 @@ static void tcpci_remove(struct i2c_client *client)
 	struct tcpci_chip *chip = i2c_get_clientdata(client);
 	int err;
 
-	/* Disable chip interrupts before unregistering port */
+	/*
+	 * The port's kthread worker is destroyed by tcpci_unregister_port()
+	 * below, while the threaded IRQ handler can still queue work on it.
+	 * Publish a zero software mask first: tcpci_irq() then ignores any
+	 * alert that is still latched, or asserted again on a shared IRQ,
+	 * even if the hardware mask write fails.  synchronize_irq() waits
+	 * for handlers that already read the old mask.
+	 */
+	tcpci_quiesce_alerts(chip->tcpci);
 	err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, 0);
 	if (err < 0)
 		dev_warn(&client->dev, "Failed to disable irqs (%pe)\n", ERR_PTR(err));
+	synchronize_irq(client->irq);
 
 	tcpci_unregister_port(chip->tcpci);
 }
-- 
2.34.1

Re: [PATCH v3] usb: typec: tcpci: filter masked alerts and quiesce the IRQ

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] usb: typec: tcpci: masking status before clearing TCPC_ALERT
  causes IRQ storm
- [Medium] usb: typec: tcpci: zeroing alert_mask before hardware mask
  write causes IRQ storm
--

commit 3b5f8c5cf280be1359b54eb345cc21c55eeb3247
Author: Yaohan Li <shunsheng1218@gmail.com>

usb: typec: tcpci: filter masked alerts and quiesce the IRQ

This commit filters TCPC_ALERT status bits with tcpci->alert_mask in
tcpci_irq() and adds an alerts_quiesced state and synchronize_irq() calls
to quiesce interrupts before unregistering the TCPM port on removal and
probe failure.

> diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> index c6165466c54b6..d413ebca75da2 100644
> --- a/drivers/usb/typec/tcpm/tcpci.c
> +++ b/drivers/usb/typec/tcpm/tcpci.c

[ ... ]

> @@ -722,8 +738,12 @@ irqreturn_t tcpci_irq(struct tcpci *tcpci)
>  	int irq_ret;
>  	unsigned int raw;
>  
> +	if (!READ_ONCE(tcpci->alert_mask))
> +		return IRQ_NONE;
> +
>  	tcpci_read16(tcpci, TCPC_ALERT, &status);
> -	irq_ret = status & tcpci->alert_mask;
> +	status &= READ_ONCE(tcpci->alert_mask);
> +	irq_ret = status;
>  
>  process_status:
>  	/*

[Severity: High]
Can masking status with tcpci->alert_mask before writing TCPC_ALERT leave
hardware-enabled alerts latched and cause a level-triggered IRQ storm?

tcpci_init() never sets TCPC_ALERT_RX_BUF_OVF, TCPC_ALERT_FAULT or
TCPC_ALERT_VBUS_DISCNCT in tcpci->alert_mask, but vendor drivers that
delegate interrupt handling to tcpci_irq() unmask those bits in hardware:

drivers/usb/typec/tcpm/tcpci_rt1711h.c:rt1711h_probe() {
    ...
	const u16 alert_mask = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_DISCARDED |
			       TCPC_ALERT_TX_FAILED | TCPC_ALERT_RX_HARD_RST |
			       TCPC_ALERT_RX_STATUS | TCPC_ALERT_POWER_STATUS |
			       TCPC_ALERT_CC_STATUS | TCPC_ALERT_RX_BUF_OVF |
			       TCPC_ALERT_FAULT;
    ...
	ret = rt1711h_write16(chip, TCPC_ALERT_MASK, alert_mask);
}

Previously, tcpci_write16(tcpci, TCPC_ALERT, status & ~TCPC_ALERT_RX_STATUS)
cleared any latched bit read from TCPC_ALERT.  With status masked by
tcpci->alert_mask first (and again after the re-read at the bottom of
tcpci_irq()), TCPC_ALERT_FAULT or TCPC_ALERT_RX_BUF_OVF is never written
back to TCPC_ALERT, so the active-low Alert# line stays asserted while
tcpci_irq() repeatedly returns IRQ_NONE until the spurious IRQ detector
disables the interrupt line.

[ ... ]

> @@ -988,10 +1013,19 @@ static void tcpci_remove(struct i2c_client *client)
>  	struct tcpci_chip *chip = i2c_get_clientdata(client);
>  	int err;
>  
> -	/* Disable chip interrupts before unregistering port */
> +	/*
> +	 * The port's kthread worker is destroyed by tcpci_unregister_port()
> +	 * below, while the threaded IRQ handler can still queue work on it.
> +	 * Publish a zero software mask first: tcpci_irq() then ignores any
> +	 * alert that is still latched, or asserted again on a shared IRQ,
> +	 * even if the hardware mask write fails.  synchronize_irq() waits
> +	 * for handlers that already read the old mask.
> +	 */
> +	tcpci_quiesce_alerts(chip->tcpci);
>  	err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, 0);
>  	if (err < 0)
>  		dev_warn(&client->dev, "Failed to disable irqs (%pe)\n", ERR_PTR(err));
> +	synchronize_irq(client->irq);
>  
>  	tcpci_unregister_port(chip->tcpci);
>  }

[Severity: Medium]
Can zeroing tcpci->alert_mask before masking TCPC_ALERT_MASK in hardware
trigger an unhandled IRQ storm if an alert fires during the I2C write?

CPU0                              irq/client->irq (SCHED_FIFO)
tcpci_remove()
  tcpci_quiesce_alerts()
    tcpci->alert_mask = 0
  tcpci_write16(..., TCPC_ALERT_MASK, 0)
    <blocks on I2C transfer>
                                  <Alert# asserted low>
                                  _tcpci_irq()
                                    tcpci_irq()
                                      if (!READ_ONCE(tcpci->alert_mask))
                                        return IRQ_NONE;
                                  irq_finalize_oneshot()
                                    <unmasks level-low IRQ line>
                                  <Alert# re-fires immediately>  <-- IRQ storm

While tcpci_write16() sleeps waiting for the I2C transfer, hardware
interrupts are still enabled in TCPC_ALERT_MASK.  Because tcpci_irq() now
returns IRQ_NONE immediately without sleeping on I2C or clearing
TCPC_ALERT, irq_finalize_oneshot() unmasks the level-triggered line with
Alert# still asserted low, spinning in the real-time IRQ thread until the
I2C write finishes or the spurious IRQ detector disables the shared IRQ.
Should TCPC_ALERT_MASK be cleared in hardware before calling
tcpci_quiesce_alerts()?

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