On Mon, Jul 31, 2023 at 7:06 PM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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/tcpm.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, struct 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->partner,
> > PD_VDO_SVDM_VER(p[0]));
> > svdm_version = 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=--)
[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
