| Message ID | 20230906084842.1922052-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f26a679ed799deef9e2934a6b60b8f38bdbf4921 |
| Headers | show |
| Series | usb: typec: ucsi: Fix NULL pointer dereference | expand |
On Wed, Sep 06, 2023 at 11:48:42AM +0300, Heikki Krogerus wrote: > Making sure the UCSI debugfs entry actually exists before > attempting to remove it. > > Fixes: df0383ffad64 ("usb: typec: ucsi: Add debugfs for ucsi commands") > Reported-by: Dave Hansen <dave.hansen@intel.com> > Closes: https://lore.kernel.org/linux-usb/700df3c4-2f6c-85f9-6c61-065bc5b2db3a@intel.com/ > Suggested-by: Dave Hansen <dave.hansen@intel.com> > Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > Cc: Saranya Gopal <saranya.gopal@intel.com> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/debugfs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c > index 0c7bf88d4a7f..f67733cecfdf 100644 > --- a/drivers/usb/typec/ucsi/debugfs.c > +++ b/drivers/usb/typec/ucsi/debugfs.c > @@ -84,6 +84,9 @@ void ucsi_debugfs_register(struct ucsi *ucsi) > > void ucsi_debugfs_unregister(struct ucsi *ucsi) > { > + if (IS_ERR_OR_NULL(ucsi) || !ucsi->debugfs) > + return; > + > debugfs_remove_recursive(ucsi->debugfs->dentry); Why are you saving the dentry at all? Why not just have debugfs look it up when you want to remove it based on the filename? Anyway, not a big deal here, just a comment. I'll queue this up after -rc1 is out. thanks, greg k-h
On 9/6/2023 03:57, Greg Kroah-Hartman wrote: > On Wed, Sep 06, 2023 at 11:48:42AM +0300, Heikki Krogerus wrote: >> Making sure the UCSI debugfs entry actually exists before >> attempting to remove it. >> >> Fixes: df0383ffad64 ("usb: typec: ucsi: Add debugfs for ucsi commands") >> Reported-by: Dave Hansen <dave.hansen@intel.com> >> Closes: https://lore.kernel.org/linux-usb/700df3c4-2f6c-85f9-6c61-065bc5b2db3a@intel.com/ >> Suggested-by: Dave Hansen <dave.hansen@intel.com> >> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> >> Cc: Saranya Gopal <saranya.gopal@intel.com> >> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> I could also reproduce it with 7733171926cc336ddf0c8f847eefaff569dbff86 on two different laptops. >> --- >> drivers/usb/typec/ucsi/debugfs.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c >> index 0c7bf88d4a7f..f67733cecfdf 100644 >> --- a/drivers/usb/typec/ucsi/debugfs.c >> +++ b/drivers/usb/typec/ucsi/debugfs.c >> @@ -84,6 +84,9 @@ void ucsi_debugfs_register(struct ucsi *ucsi) >> >> void ucsi_debugfs_unregister(struct ucsi *ucsi) >> { >> + if (IS_ERR_OR_NULL(ucsi) || !ucsi->debugfs) >> + return; >> + >> debugfs_remove_recursive(ucsi->debugfs->dentry); > > Why are you saving the dentry at all? Why not just have debugfs look it > up when you want to remove it based on the filename? > > Anyway, not a big deal here, just a comment. I'll queue this up after > -rc1 is out. It might be worth sending before rc1 if it ends up being widespread. > > thanks, > > greg k-h
On 06.09.23 14:05, Mario Limonciello wrote: > On 9/6/2023 03:57, Greg Kroah-Hartman wrote: >> On Wed, Sep 06, 2023 at 11:48:42AM +0300, Heikki Krogerus wrote: >>> Making sure the UCSI debugfs entry actually exists before >>> attempting to remove it. >>> >>> Fixes: df0383ffad64 ("usb: typec: ucsi: Add debugfs for ucsi commands") >>> Reported-by: Dave Hansen <dave.hansen@intel.com> >>> Closes: >>> https://lore.kernel.org/linux-usb/700df3c4-2f6c-85f9-6c61-065bc5b2db3a@intel.com/ >>> Suggested-by: Dave Hansen <dave.hansen@intel.com> >>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> >>> Cc: Saranya Gopal <saranya.gopal@intel.com> >>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > I could also reproduce it with 7733171926cc336ddf0c8f847eefaff569dbff86 > on two different laptops. > >>> --- >>> drivers/usb/typec/ucsi/debugfs.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/usb/typec/ucsi/debugfs.c >>> b/drivers/usb/typec/ucsi/debugfs.c >>> index 0c7bf88d4a7f..f67733cecfdf 100644 >>> --- a/drivers/usb/typec/ucsi/debugfs.c >>> +++ b/drivers/usb/typec/ucsi/debugfs.c >>> @@ -84,6 +84,9 @@ void ucsi_debugfs_register(struct ucsi *ucsi) >>> void ucsi_debugfs_unregister(struct ucsi *ucsi) >>> { >>> + if (IS_ERR_OR_NULL(ucsi) || !ucsi->debugfs) >>> + return; >>> + >>> debugfs_remove_recursive(ucsi->debugfs->dentry); >> >> Why are you saving the dentry at all? Why not just have debugfs look it >> up when you want to remove it based on the filename? >> >> Anyway, not a big deal here, just a comment. I'll queue this up after >> -rc1 is out. > > It might be worth sending before rc1 if it ends up being widespread. FWIW, as someone that afaics just ran into the regression fixed by this patch with -rc1, I would have liked that patch to be in -rc1 as well... Ciao, Thorsten
On Mon, Sep 11, 2023 at 11:46:17AM +0200, Thorsten Leemhuis wrote: > On 06.09.23 14:05, Mario Limonciello wrote: > > On 9/6/2023 03:57, Greg Kroah-Hartman wrote: > >> On Wed, Sep 06, 2023 at 11:48:42AM +0300, Heikki Krogerus wrote: > >>> Making sure the UCSI debugfs entry actually exists before > >>> attempting to remove it. > >>> > >>> Fixes: df0383ffad64 ("usb: typec: ucsi: Add debugfs for ucsi commands") > >>> Reported-by: Dave Hansen <dave.hansen@intel.com> > >>> Closes: > >>> https://lore.kernel.org/linux-usb/700df3c4-2f6c-85f9-6c61-065bc5b2db3a@intel.com/ > >>> Suggested-by: Dave Hansen <dave.hansen@intel.com> > >>> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> > >>> Cc: Saranya Gopal <saranya.gopal@intel.com> > >>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > I could also reproduce it with 7733171926cc336ddf0c8f847eefaff569dbff86 > > on two different laptops. > > > >>> --- > >>> drivers/usb/typec/ucsi/debugfs.c | 3 +++ > >>> 1 file changed, 3 insertions(+) > >>> > >>> diff --git a/drivers/usb/typec/ucsi/debugfs.c > >>> b/drivers/usb/typec/ucsi/debugfs.c > >>> index 0c7bf88d4a7f..f67733cecfdf 100644 > >>> --- a/drivers/usb/typec/ucsi/debugfs.c > >>> +++ b/drivers/usb/typec/ucsi/debugfs.c > >>> @@ -84,6 +84,9 @@ void ucsi_debugfs_register(struct ucsi *ucsi) > >>> void ucsi_debugfs_unregister(struct ucsi *ucsi) > >>> { > >>> + if (IS_ERR_OR_NULL(ucsi) || !ucsi->debugfs) > >>> + return; > >>> + > >>> debugfs_remove_recursive(ucsi->debugfs->dentry); > >> > >> Why are you saving the dentry at all? Why not just have debugfs look it > >> up when you want to remove it based on the filename? > >> > >> Anyway, not a big deal here, just a comment. I'll queue this up after > >> -rc1 is out. > > > > It might be worth sending before rc1 if it ends up being widespread. > > FWIW, as someone that afaics just ran into the regression fixed by this > patch with -rc1, I would have liked that patch to be in -rc1 as well... It's now queued up and will go to Linus this week. thanks, greg k-h
diff --git a/drivers/usb/typec/ucsi/debugfs.c b/drivers/usb/typec/ucsi/debugfs.c index 0c7bf88d4a7f..f67733cecfdf 100644 --- a/drivers/usb/typec/ucsi/debugfs.c +++ b/drivers/usb/typec/ucsi/debugfs.c @@ -84,6 +84,9 @@ void ucsi_debugfs_register(struct ucsi *ucsi) void ucsi_debugfs_unregister(struct ucsi *ucsi) { + if (IS_ERR_OR_NULL(ucsi) || !ucsi->debugfs) + return; + debugfs_remove_recursive(ucsi->debugfs->dentry); kfree(ucsi->debugfs); }
Making sure the UCSI debugfs entry actually exists before attempting to remove it. Fixes: df0383ffad64 ("usb: typec: ucsi: Add debugfs for ucsi commands") Reported-by: Dave Hansen <dave.hansen@intel.com> Closes: https://lore.kernel.org/linux-usb/700df3c4-2f6c-85f9-6c61-065bc5b2db3a@intel.com/ Suggested-by: Dave Hansen <dave.hansen@intel.com> Suggested-by: Mario Limonciello <mario.limonciello@amd.com> Cc: Saranya Gopal <saranya.gopal@intel.com> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/typec/ucsi/debugfs.c | 3 +++ 1 file changed, 3 insertions(+)