[PATCH 0/2] usb: typec: ucsi: fix input validation in UCSI core

[PATCH 0/2] usb: typec: ucsi: fix input validation in UCSI core

[Nathan Rebello]

Two input validation fixes for the UCSI core driver:

Patch 1 adds a bounds check on the connector number in
ucsi_connector_change(). The connector number is extracted from the CCI
register (7-bit field, range 1-127) but is used to index the connector
array without validation. A malicious or malfunctioning PPM could cause
an out-of-bounds access.

Patch 2 clamps the return value of ucsi_run_command() to the caller's
buffer size. The current code returns UCSI_CCI_LENGTH() directly from
the CCI register, which may exceed the buffer provided by the caller,
leading to out-of-bounds reads in callers like ucsi_register_altmodes().

Both issues were found via static analysis and confirmed with
libFuzzer and AddressSanitizer.

Nathan Rebello (2):
  usb: typec: ucsi: validate connector number in ucsi_connector_change()
  usb: typec: ucsi: clamp returned length in ucsi_run_command()

 drivers/usb/typec/ucsi/ucsi.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

-- 
2.43.0.windows.1

[PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()

[Nathan Rebello]

ucsi_connector_change() uses the connector number from the CCI as an
index into the connector array without first verifying it falls within
the valid range. The connector number is extracted from the CCI register
via UCSI_CCI_CONNECTOR(), which returns a 7-bit value (1-127), but the
connector array is typically only 2-4 entries.

A malicious or malfunctioning device could report an out-of-range
connector number, causing an out-of-bounds array access.

Add a bounds check to reject invalid connector numbers before indexing.

Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
Signed-off-by: Nathan Rebello <nathan.c.rebello@gmail.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index a7b388dc7fa0..7109d3bd39b4 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -1345,7 +1345,14 @@ static void ucsi_handle_connector_change(struct work_struct *work)
  */
 void ucsi_connector_change(struct ucsi *ucsi, u8 num)
 {
-	struct ucsi_connector *con = &ucsi->connector[num - 1];
+	struct ucsi_connector *con;
+
+	if (num < 1 || num > ucsi->cap.num_connectors) {
+		dev_warn(ucsi->dev, "invalid connector number %d\n", num);
+		return;
+	}
+
+	con = &ucsi->connector[num - 1];
 
 	if (!(ucsi->ntfy & UCSI_ENABLE_NTFY_CONNECTOR_CHANGE)) {
 		dev_dbg(ucsi->dev, "Early connector change event\n");
-- 
2.43.0.windows.1

Re: [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()

[Greg KH]

On Thu, Feb 19, 2026 at 11:49:23AM -0500, Nathan Rebello wrote:
> ucsi_connector_change() uses the connector number from the CCI as an
> index into the connector array without first verifying it falls within
> the valid range. The connector number is extracted from the CCI register
> via UCSI_CCI_CONNECTOR(), which returns a 7-bit value (1-127), but the
> connector array is typically only 2-4 entries.
> 
> A malicious or malfunctioning device could report an out-of-range
> connector number, causing an out-of-bounds array access.

Is this before, or after, the device has been bound to the driver?

thanks,

greg k-h

Re: [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()

[Nathan Rebello]

After. ucsi_connector_change() is only called via ucsi_notify_common(),
which processes CCI interrupts from a bound device. The connector number
comes from the CCI register (UCSI_CCI_CONNECTOR), a 7-bit field that
can return 1-127, while the connector array is allocated based on
num_connectors (typically 2-4). The same register is already validated
for other fields in ucsi_init() (line 1840: "This is out of spec but
happens in buggy FW").

Nathan

Re: [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()

[Greg KH]

On Fri, Feb 20, 2026 at 01:34:04AM -0500, Nathan Rebello wrote:
> After.

I'm sorry, but I have no context here.  Remember, some of us get 1000+
emails a day, context matters :)

/me goes and digs...

> ucsi_connector_change() is only called via ucsi_notify_common(),
> which processes CCI interrupts from a bound device. The connector number
> comes from the CCI register (UCSI_CCI_CONNECTOR), a 7-bit field that
> can return 1-127, while the connector array is allocated based on
> num_connectors (typically 2-4). The same register is already validated
> for other fields in ucsi_init() (line 1840: "This is out of spec but
> happens in buggy FW").

Why not fix the buggy fw?  :)

Remember, Linux trusts hardware.  If people wish to change that model,
great, I'm all for that, and wack-a-mole fixes like this for after a
driver is bound to a device are nice to attempt, and clean up, but are
not really the solution.

That's why we have USBGuard, which prevents the driver from binding to
the device.  Make that policy decision in userspace, and then if you
trust the device, great, we trust the device to send us correct data.

And again, fix the hardware to not send invalid data.

thanks,

greg k-h

Re: [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()

[Greg KH]

On Thu, Feb 19, 2026 at 11:49:23AM -0500, Nathan Rebello wrote:
> ucsi_connector_change() uses the connector number from the CCI as an
> index into the connector array without first verifying it falls within
> the valid range. The connector number is extracted from the CCI register
> via UCSI_CCI_CONNECTOR(), which returns a 7-bit value (1-127), but the
> connector array is typically only 2-4 entries.
> 
> A malicious or malfunctioning device could report an out-of-range
> connector number, causing an out-of-bounds array access.
> 
> Add a bounds check to reject invalid connector numbers before indexing.
> 
> Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
> Signed-off-by: Nathan Rebello <nathan.c.rebello@gmail.com>
> ---
>  drivers/usb/typec/ucsi/ucsi.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index a7b388dc7fa0..7109d3bd39b4 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1345,7 +1345,14 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>   */
>  void ucsi_connector_change(struct ucsi *ucsi, u8 num)
>  {
> -	struct ucsi_connector *con = &ucsi->connector[num - 1];
> +	struct ucsi_connector *con;
> +
> +	if (num < 1 || num > ucsi->cap.num_connectors) {
> +		dev_warn(ucsi->dev, "invalid connector number %d\n", num);
> +		return;

Shouldn't this return an error and have the caller stop what it was
attempting to do?  When this is called in ucsi_init(), the
num_connectors is already parsed, so how can this be wrong?  Shouldn't
these values all be verified in one single place and then if any of the
descriptors are "incorrect", the device rejected at that point in time
and not require "deep" changes in the logic here to try to find these
types of errors?

in short, let's validate the device once, and after that is done, we can
"trust" that it is correct and not need to check this stuff all over the
place.

thanks,

greg k-h

Re: [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()

[Nathan Rebello]

On Wed, 11 Mar 2026 at 14:10:28 +0100, Greg KH wrote:
> Shouldn't this return an error and have the caller stop what it was
> attempted to do?  When this is called in ucsi_init(), the
> num_connectors is already parsed, so how can this be wrong?  Shouldn't
> these values all be verified in one single place and then if any of the
> descriptors are "incorrect", the device rejected at that point in time
> and not require "deep" changes in the logic here to try to find these
> types of errors?
>
> in short, let's validate the device once, and after that is done, we can
> "trust" that it is correct and not need to check this stuff all over the
> place.

The CCI connector number can't be validated at init because it's not a
static descriptor -- it arrives fresh with each runtime interrupt from
the CCI register. The device can correctly report num_connectors=2 at
init time, then later send an interrupt with connector_number=50 in the
CCI. This is the same class of issue already handled at line 1840 in
ucsi_init(), where buggy firmware sets a reserved bit in num_connectors
and a defensive check was added because it "happens in buggy FW."

Without a bounds check, an out-of-range connector number indexes past
the connector array into adjacent heap memory, and the resulting pointer
is passed to schedule_work() -- dereferencing whatever function pointer
happens to sit at that offset. A single buggy interrupt could crash the
kernel.

The closest single validation point is ucsi_notify_common(), the sole
entry point where the CCI is parsed before calling
ucsi_connector_change(). A one-line bounds check there would cover all
backends and all call sites.

Happy to send a v2 with that approach if acceptable.

Nathan Rebello

Re: [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()

[Greg KH]

On Wed, Mar 11, 2026 at 05:49:36PM -0400, Nathan Rebello wrote:
> On Wed, 11 Mar 2026 at 14:10:28 +0100, Greg KH wrote:
> > Shouldn't this return an error and have the caller stop what it was
> > attempted to do?  When this is called in ucsi_init(), the
> > num_connectors is already parsed, so how can this be wrong?  Shouldn't
> > these values all be verified in one single place and then if any of the
> > descriptors are "incorrect", the device rejected at that point in time
> > and not require "deep" changes in the logic here to try to find these
> > types of errors?
> >
> > in short, let's validate the device once, and after that is done, we can
> > "trust" that it is correct and not need to check this stuff all over the
> > place.
> 
> The CCI connector number can't be validated at init because it's not a
> static descriptor -- it arrives fresh with each runtime interrupt from
> the CCI register. The device can correctly report num_connectors=2 at
> init time, then later send an interrupt with connector_number=50 in the
> CCI. This is the same class of issue already handled at line 1840 in
> ucsi_init(), where buggy firmware sets a reserved bit in num_connectors
> and a defensive check was added because it "happens in buggy FW."
> 
> Without a bounds check, an out-of-range connector number indexes past
> the connector array into adjacent heap memory, and the resulting pointer
> is passed to schedule_work() -- dereferencing whatever function pointer
> happens to sit at that offset. A single buggy interrupt could crash the
> kernel.
> 
> The closest single validation point is ucsi_notify_common(), the sole
> entry point where the CCI is parsed before calling
> ucsi_connector_change(). A one-line bounds check there would cover all
> backends and all call sites.
> 
> Happy to send a v2 with that approach if acceptable.

Yes, please do that as we need central points where data is validated to
help keep the code sane.

thanks,

greg k-h

Re: [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()

[Nathan Rebello]

On Thu, 12 Mar 2026 at 06:03:33 +0100, Greg KH wrote:
> Yes, please do that as we need central points where data is validated to
> help keep the code sane.

Thanks. On closer inspection, ucsi_notify_common() is not actually the
sole entry point -- ucsi_register() also calls ucsi_connector_change()
directly when reading CCI at the end of init, and ucsi_yoga_c630.c
calls it from its own notifier.

The true single central point is ucsi_connector_change() itself.  v2
adds the bounds check there, before the array dereference, which covers
all callers.

Will send v2 shortly.

Nathan Rebello

[PATCH 2/2] usb: typec: ucsi: clamp returned length in ucsi_run_command()

[Nathan Rebello]

ucsi_run_command() returns UCSI_CCI_LENGTH(cci) as the data length to
callers. This value comes directly from the CCI register and reflects
the length reported by the PPM firmware, not the actual number of bytes
copied into the caller's buffer (which is bounded by the size parameter
passed to sync_control).

If the firmware reports a length larger than the buffer provided by the
caller, callers such as ucsi_register_altmodes() will iterate past the
end of their stack buffer when computing:

    num = len / sizeof(alt[0]);
    for (j = 0; j < num; j++)  /* reads past alt[2] */

Clamp the return value to the buffer size to prevent out-of-bounds reads.

Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
Signed-off-by: Nathan Rebello <nathan.c.rebello@gmail.com>
---
 drivers/usb/typec/ucsi/ucsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 7109d3bd39b4..5791597fe662 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -140,7 +140,7 @@ static int ucsi_run_command(struct ucsi *ucsi, u64 command, u32 *cci,
 	if (ret)
 		return ret;
 
-	return err ?: UCSI_CCI_LENGTH(*cci);
+	return err ?: min_t(u32, UCSI_CCI_LENGTH(*cci), size);
 }
 
 static int ucsi_read_error(struct ucsi *ucsi, u8 connector_num)
-- 
2.43.0.windows.1