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

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]

Message-ID: <CAMRc=McRiv6vtU5LySrkbfdbCaqZ2tXbHnazo4iahRS-SKAXxg@mail.gmail.com>
Date: Thu, 18 Apr 2024 02:23:38 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Daniel Okazaki <dtokazaki@...gle.com>
Cc: Arnd Bergmann <arnd@...db.de>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, kernel-team@...roid.com, 
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] at24: fix memory corruption race condition

On Thu, Apr 18, 2024 at 1:59 AM Bartosz Golaszewski <brgl@...ev.pl> wrote:
>
> On Thu, Apr 18, 2024 at 1:07 AM Daniel Okazaki <dtokazaki@...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@...gle.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 *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