Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230630065711.801569-1-hhhuuu@google.com> <ZMeVnbnIuf1bNuKQ@kuha.fi.intel.com>
In-Reply-To: <ZMeVnbnIuf1bNuKQ@kuha.fi.intel.com>
From:   Jimmy Hu <hhhuuu@google.com>
Date:   Tue, 1 Aug 2023 11:21:36 +0800
Message-ID: <CAJh=zjJBUb2JpEh4Tf6z0V9KTy2jufRQktuJt2oMYbeu52PXzg@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: tcpm: Add IS_ERR_OR_NULL check for port->partner
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     linux@roeck-us.net, gregkh@linuxfoundation.org, kyletso@google.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 31, 2023 at 7:06=E2=80=AFPM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi,
>
> I'm sorry to keep you waiting.
>
> On Fri, Jun 30, 2023 at 06:57:11AM +0000, Jimmy Hu wrote:
> > port->partner may be an error or NULL, so we must check it with
> > IS_ERR_OR_NULL() before dereferencing it.
>
> Have you seen this happening? Maybe the partner check should happen
> earlier, before tcpm_pd_svdm() is even called?
>
> > Fixes: 5e1d4c49fbc8 ("usb: typec: tcpm: Determine common SVDM Version")
> > Signed-off-by: Jimmy Hu <hhhuuu@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcp=
m.c
> > index 829d75ebab42..cd2590eead04 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -1626,6 +1626,8 @@ static int tcpm_pd_svdm(struct tcpm_port *port, s=
truct typec_altmode *adev,
> >                               break;
> >
> >                       if (PD_VDO_SVDM_VER(p[0]) < svdm_version) {
> > +                             if (IS_ERR_OR_NULL(port->partner))
> > +                                     break;
> >                               typec_partner_set_svdm_version(port->part=
ner,
> >                                                              PD_VDO_SVD=
M_VER(p[0]));
> >                               svdm_version =3D PD_VDO_SVDM_VER(p[0]);
>
> Now you will still build a response? I'm pretty sure you don't want
> that.
>
> Do we need to do anything in this function if the partner is lost? If
> not, then why not just check the partner in the beginning of the
> function. Or just make sure we don't even call tcpm_pd_svdm() if
> there's no partner.
>
> thanks,
>
> --
> heikki

Yes, we've seen this. Here is part of the last kmsg.

[978098.478754][  T319] typec port0: failed to register partner (-17)
...
[978101.505668][  T319] Unable to handle kernel NULL pointer
dereference at virtual address 000000000000039f
[978101.864439][  T319] CPU: 5 PID: 319 Comm: i2c-max77759tcp Tainted:
G        W  O      5.10.66-android12-9-00229-gd736cbf8d9ac-ab7921439
#1
[978101.866919][ T1176] [07:31:46.926532][dhd][wlan]
[978101.876939][  T319] Hardware name: Raven DVT (DT)
[978101.876949][  T319] pstate: 80c00005 (Nzcv daif +PAN +UAO -TCO BTYPE=3D=
--)
[978101.876982][  T319] pc : tcpm_pd_data_request+0x310/0x13fc
[978101.876987][  T319] lr : tcpm_pd_data_request+0x298/0x13fc
...
978101.886481][  T319] Call trace:
[978101.886492][  T319]  tcpm_pd_data_request+0x310/0x13fc
[978101.886506][  T319]  tcpm_pd_rx_handler+0x100/0x9e8
[978101.898747][  T319]  kthread_worker_fn+0x178/0x58c
[978101.898756][  T319]  kthread+0x150/0x200
[978101.898769][  T319]  ret_from_fork+0x10/0x30

Since this happens when PD_VDO_SVDM_VER(p[0]) < svdm_version, I think
we can check the partner after the condition is met.
Or check it when entering CMD_DISCOVER_IDENT case. Just like:
case CMDT_RSP_ACK:
/* silently drop message if we are not connected */
if (IS_ERR_OR_NULL(port->partner))
break;

Thanks,
Jimmy