Re: [PATCH v1] at24: fix memory corruption race condition

[Daniel Okazaki]

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


On Wed, Apr 17, 2024 at 5:23 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> On Thu, Apr 18, 2024 at 1:59 AM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> >
> > On Thu, Apr 18, 2024 at 1:07 AM Daniel Okazaki <dtokazaki@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > >  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 *client)
> > >         }
> > >         pm_runtime_enable(dev);
> > >
> > > -       at24->nvmem = 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 probe (i.e.
> > > @@ -782,6 +773,15 @@ static int at24_probe(struct i2c_client *client)
> > >                 }
> > >         }
> > >
> > > +       at24->nvmem = 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 == &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