Received-SPF: pass (google.com: domain of linux-kernel+bounces-150560-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240417230637.2592473-1-dtokazaki@google.com>
 <CAMRc=Mcbi8BQ-tHNs+BVjbm3aDSVY0zzOM4QfND3d2y8ryT62g@mail.gmail.com>
 <CAMRc=McRiv6vtU5LySrkbfdbCaqZ2tXbHnazo4iahRS-SKAXxg@mail.gmail.com> <CAOC40965VyOQX_WHxA0a=Ke6knoeYYYjqTkFva9eNyG55_Yi-g@mail.gmail.com>
In-Reply-To: <CAOC40965VyOQX_WHxA0a=Ke6knoeYYYjqTkFva9eNyG55_Yi-g@mail.gmail.com>
From: Bartosz Golaszewski <brgl@bgdev.pl>
Date: Thu, 18 Apr 2024 19:17:00 +0200
Message-ID: <CAMRc=Mcb0uAjUBOKraLaev_xU+3OObj9YJm1Yq2A2s2bZOViiQ@mail.gmail.com>
Subject: Re: [PATCH v1] at24: fix memory corruption race condition
To: Daniel Okazaki <dtokazaki@google.com>
Cc: Arnd Bergmann <arnd@arndb.de>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, kernel-team@android.com, 
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Apr 18, 2024 at 7:13=E2=80=AFPM Daniel Okazaki <dtokazaki@google.co=
m> wrote:
>
> nvmem devices allow for linking by name in the DTS which doesn't
> create a dependency in the probe order.
>
> What happens is driver B probe starts shortly after the eeprom
> probe and calls of_nvmem_device_get. Since a device is
> registered it starts using it; however if the eeprom isn't there
> then the read will fail and it will start tearing down the resources.
> Driver B will now access invalid memory causing a kernel panic.
>
> Daniel
>

Please don't top-post on the linux kernel mailing list.

I'm Cc'ing Srini, the maintainer of NVMEM. I think this is an issue
with nvmem core as it shouldn't allow access to nvmem devices once it
starts tearing them down. Srini, could you comment on this?

Bartosz

>
> On Wed, Apr 17, 2024 at 5:23=E2=80=AFPM Bartosz Golaszewski <brgl@bgdev.p=
l> wrote:
> >
> > On Thu, Apr 18, 2024 at 1:59=E2=80=AFAM Bartosz Golaszewski <brgl@bgdev=
pl> wrote:
> > >
> > > On Thu, Apr 18, 2024 at 1:07=E2=80=AFAM Daniel Okazaki <dtokazaki@goo=
gle.com> wrote:
> > > >
> > > > If the eeprom is not accessible, an nvmem device will be registered=
, the
> > > > read will fail, and the device will be torn down. If another driver
> > > > accesses the nvmem device after the teardown, it will reference
> > > > invalid memory.
> > > >
> > > > Move the failure point before registering the nvmem device.
> > > >
> > > > Signed-off-by: Daniel Okazaki <dtokazaki@google.com>
> > > > ---
> > > >  drivers/misc/eeprom/at24.c | 18 +++++++++---------
> > > >  1 file changed, 9 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.=
c
> > > > index 572333ead5fb..4bd4f32bcdab 100644
> > > > --- a/drivers/misc/eeprom/at24.c
> > > > +++ b/drivers/misc/eeprom/at24.c
> > > > @@ -758,15 +758,6 @@ static int at24_probe(struct i2c_client *clien=
t)
> > > >         }
> > > >         pm_runtime_enable(dev);
> > > >
> > > > -       at24->nvmem =3D devm_nvmem_register(dev, &nvmem_config);
> > > > -       if (IS_ERR(at24->nvmem)) {
> > > > -               pm_runtime_disable(dev);
> > > > -               if (!pm_runtime_status_suspended(dev))
> > > > -                       regulator_disable(at24->vcc_reg);
> > > > -               return dev_err_probe(dev, PTR_ERR(at24->nvmem),
> > > > -                                    "failed to register nvmem\n");
> > > > -       }
> > > > -
> > > >         /*
> > > >          * Perform a one-byte test read to verify that the chip is =
functional,
> > > >          * unless powering on the device is to be avoided during pr=
obe (i.e.
> > > > @@ -782,6 +773,15 @@ static int at24_probe(struct i2c_client *clien=
t)
> > > >                 }
> > > >         }
> > > >
> > > > +       at24->nvmem =3D devm_nvmem_register(dev, &nvmem_config);
> > > > +       if (IS_ERR(at24->nvmem)) {
> > > > +               pm_runtime_disable(dev);
> > > > +               if (!pm_runtime_status_suspended(dev))
> > > > +                       regulator_disable(at24->vcc_reg);
> > > > +               return dev_err_probe(dev, PTR_ERR(at24->nvmem),
> > > > +                                    "failed to register nvmem\n");
> > > > +       }
> > > > +
> > > >         /* If this a SPD EEPROM, probe for DDR3 thermal sensor */
> > > >         if (cdata =3D=3D &at24_data_spd)
> > > >                 at24_probe_temp_sensor(client);
> > > > --
> > > > 2.44.0.683.g7961c838ac-goog
> > > >
> > >
> > > Looks good, can you add a Fixes tag?
> > >
> > > Thanks,
> > > Bartosz
> >
> > Wait... While the patch is still correct - we shouldn't needlessly
> > create the nvmem device - why would anything crash? Looks like a
> > problem with nvmem then? How did you trigger this issue?
> >
> > Bart