[PATCH] usb: typec: tcpm: Add IS_ERR_OR_NULL check for port->partner

[PATCH] usb: typec: tcpm: Add IS_ERR_OR_NULL check for port->partner

[Jimmy Hu]

port->partner may be an error or NULL, so we must check it with
IS_ERR_OR_NULL() before dereferencing it.

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/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]);
-- 
2.41.0.255.g8b1d071c50-goog

Re: [PATCH] usb: typec: tcpm: Add IS_ERR_OR_NULL check for port->partner

[Heikki Krogerus]

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/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

Re: [PATCH] usb: typec: tcpm: Add IS_ERR_OR_NULL check for port->partner

[Jimmy Hu]

On Mon, Jul 31, 2023 at 7:06 PM 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/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