| Message ID | 20230630065711.801569-1-hhhuuu@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tcpm: Add IS_ERR_OR_NULL check for port->partner | expand |
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,
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
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]);
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(+)