All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg KH <gregkh@linuxfoundation.org>
To: Nathan Rebello <nathan.c.rebello@gmail.com>
Cc: linux-usb@vger.kernel.org, heikki.krogerus@linux.intel.com,
	kyungtae.kim@dartmouth.edu
Subject: Re: [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change()
Date: Thu, 12 Mar 2026 06:03:33 +0100	[thread overview]
Message-ID: <2026031206-many-halt-cb1b@gregkh> (raw)
In-Reply-To: <20260311214936.1045-1-nathan.c.rebello@gmail.com>

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

  reply	other threads:[~2026-03-12  5:03 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-02-19 16:49 [PATCH 0/2] usb: typec: ucsi: fix input validation in UCSI core Nathan Rebello
2026-02-19 16:49 ` [PATCH 1/2] usb: typec: ucsi: validate connector number in ucsi_connector_change() Nathan Rebello
2026-02-20  6:09   ` Greg KH
2026-02-20  6:34   ` Nathan Rebello
2026-02-20  6:53     ` Greg KH
2026-03-11 13:10   ` Greg KH
2026-03-11 21:49     ` Nathan Rebello
2026-03-12  5:03       ` Greg KH [this message]
2026-03-12  5:44         ` Nathan Rebello
2026-02-19 16:49 ` [PATCH 2/2] usb: typec: ucsi: clamp returned length in ucsi_run_command() Nathan Rebello

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2026031206-many-halt-cb1b@gregkh \
    --to=gregkh@linuxfoundation.org \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=kyungtae.kim@dartmouth.edu \
    --cc=linux-usb@vger.kernel.org \
    --cc=nathan.c.rebello@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.