linux-kernel - Re: [PATCH v1 RESEND] spmi: hisi-spmi-controller: Fix kernel panic on rmmod

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 29 Jan 2024 08:31:39 +0100
From: Johan Hovold <johan@...nel.org>
To: VAMSHI GAJJELA <vamshigajjela@...gle.com>
Cc: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
	Stephen Boyd <sboyd@...nel.org>,
	Johan Hovold <johan+linaro@...nel.org>,
	Caleb Connolly <caleb.connolly@...aro.org>,
	linux-kernel@...r.kernel.org, manugautam@...gle.com
Subject: Re: [PATCH v1 RESEND] spmi: hisi-spmi-controller: Fix kernel panic
 on rmmod

On Mon, Jan 29, 2024 at 10:13:22AM +0530, VAMSHI GAJJELA wrote:
> On Fri, Jan 26, 2024 at 1:48 PM Johan Hovold <johan@...nel.org> wrote:
> >
> > On Fri, Jan 26, 2024 at 11:41:53AM +0530, Vamshi Gajjela wrote:
> > > Ensure consistency in spmi_controller pointers between
> > > spmi_controller_remove/put and driver spmi_del_controller functions.
> > > The former requires a pointer to struct spmi_controller, while the
> > > latter passes a pointer of struct spmi_controller_dev, leading to a
> > > "Null pointer exception".
> > >
> > > 'nr' member of struct spmi_controller, which serves as an identifier
> > > for the controller/bus. This value is assigned a dynamic ID in
> > > spmi_controller_alloc, and overriding it from the driver results in an
> > > ida_free error "ida_free called for id=xx which is not allocated".
> >
> > No Fixes tag?

> There isn't a bug, I will remove word "Fix"

Both of the issues you point out above sounds like bugs that deserve a
Fixes tag.

> > > Signed-off-by: Vamshi Gajjela <vamshigajjela@...gle.com>
> > > ---
> > >  drivers/spmi/hisi-spmi-controller.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/spmi/hisi-spmi-controller.c b/drivers/spmi/hisi-spmi-controller.c
> > > index 9cbd473487cb..af51ffe24072 100644
> > > --- a/drivers/spmi/hisi-spmi-controller.c
> > > +++ b/drivers/spmi/hisi-spmi-controller.c
> > > @@ -303,7 +303,6 @@ static int spmi_controller_probe(struct platform_device *pdev)
> > >
> > >       spin_lock_init(&spmi_controller->lock);
> > >
> > > -     ctrl->nr = spmi_controller->channel;

> This remains applicable, however, it could lead to a failure in the
> spmi_ctrl_release, I
> will refactor the patch to address this.
> also "spmi_del_controller" is removed from 6.7.2

No, this has not changed in 6.7.2, it has been removed from 6.8-rc1.

> > >       ctrl->dev.parent = pdev->dev.parent;
> > >       ctrl->dev.of_node = of_node_get(pdev->dev.of_node);
> > >
> > > @@ -326,7 +325,8 @@ static int spmi_controller_probe(struct platform_device *pdev)
> > >
> > >  static void spmi_del_controller(struct platform_device *pdev)
> >
> > This function does not exist in mainline so presumably this is some bug
> > you've introduced in your downstream driver that you're trying to fix.
> >
> > So this patch looks all bogus.

> spmi_del_controller is present until in 6.7.2, I have made this patch
> in last week of Dec,
> I should have checked before resending, apologies.

The bug you found was apparently accidentally fixed by commit
490d88ef548d ("spmi: hisi-spmi-controller: Use
devm_spmi_controller_add()") in 6.8-rc1 but I don't see any record of it
having been backported yet.

As it depends on new helper function that will likely not happen either.

Perhaps you can split your patch in two separate fixes and ask the stable
team to backport the driver-data one.

> > >  {
> > > -     struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> > > +     struct spmi_controller_dev *spmi_controller = platform_get_drvdata(pdev);
> > > +     struct spmi_controller *ctrl = spmi_controller->controller;
> > >
> > >       spmi_controller_remove(ctrl);
> > >       spmi_controller_put(ctrl);

Johan