static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
in vpd_probe() based on header's ro and rw sizes.
In vpd_remove() vpd_section_destroy() performs deinitialization based
on enabled flag, which is set to true by vpd_sections_init().
This leads to call of vpd_section_destroy() on already destroyed section
for probe-release-probe-release sequence if first probe performs
ro_vpd initialization and second probe does not initialize it.
The patch adds changing enabled flag on vpd_section_destroy.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Vasilyev <[email protected]>
---
drivers/firmware/google/vpd.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index e9db895916c3..5347c17c7108 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -246,6 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec)
sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr);
kfree(sec->raw_name);
memunmap(sec->baseaddr);
+ sec->enabled = false;
}
return 0;
--
2.18.0
On Mon, Jul 23, 2018 at 07:48:57PM +0300, Anton Vasilyev wrote:
> static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
> in vpd_probe() based on header's ro and rw sizes.
> In vpd_remove() vpd_section_destroy() performs deinitialization based
> on enabled flag, which is set to true by vpd_sections_init().
> This leads to call of vpd_section_destroy() on already destroyed section
> for probe-release-probe-release sequence if first probe performs
> ro_vpd initialization and second probe does not initialize it.
>
> The patch adds changing enabled flag on vpd_section_destroy.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Anton Vasilyev <[email protected]>
Good find.
Reviewed-by: Dmitry Torokhov <[email protected]>
> ---
> drivers/firmware/google/vpd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> index e9db895916c3..5347c17c7108 100644
> --- a/drivers/firmware/google/vpd.c
> +++ b/drivers/firmware/google/vpd.c
> @@ -246,6 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec)
> sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr);
> kfree(sec->raw_name);
> memunmap(sec->baseaddr);
> + sec->enabled = false;
> }
>
> return 0;
> --
> 2.18.0
>
Thanks.
--
Dmitry
On Mon, Jul 23, 2018 at 07:48:57PM +0300, Anton Vasilyev wrote:
> static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
> in vpd_probe() based on header's ro and rw sizes.
> In vpd_remove() vpd_section_destroy() performs deinitialization based
> on enabled flag, which is set to true by vpd_sections_init().
> This leads to call of vpd_section_destroy() on already destroyed section
> for probe-release-probe-release sequence if first probe performs
> ro_vpd initialization and second probe does not initialize it.
>
I am not sure if the situation described can be seen in the first place.
The second probe would only not perform ro_vpd initialization if it fails
prior to that, ie if it fails to allocate memory or if there is a
consistency problem. In that case the remove function would not be called.
However, there is a problem in the code: A partially failed probe will
leave the system in inconsistent state. Example: ro section initializes,
rw section fails to initialize. The probe will fail, but the ro section
will not be destroyed, its sysfs attributes still exist, and its memory
is still mapped. It would make more sense to fix _that_ problem.
Essentially, vpd_sections_init() should clean up after itself after it
fails to initialize a section.
Note that I am not convinced that the "enabled" flag is needed in the first
place. It is only relevant if vpd_section_destroy() is called, which only
happens from the remove function. The remove function is only called if the
probe function succeeded. In that case it is always set for both sections.
Thanks,
Guenter
> The patch adds changing enabled flag on vpd_section_destroy.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Anton Vasilyev <[email protected]>
> ---
> drivers/firmware/google/vpd.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> index e9db895916c3..5347c17c7108 100644
> --- a/drivers/firmware/google/vpd.c
> +++ b/drivers/firmware/google/vpd.c
> @@ -246,6 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec)
> sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr);
> kfree(sec->raw_name);
> memunmap(sec->baseaddr);
> + sec->enabled = false;
> }
>
> return 0;
> --
> 2.18.0
>
On Mon, Jul 23, 2018 at 10:13:36AM -0700, Guenter Roeck wrote:
> On Mon, Jul 23, 2018 at 07:48:57PM +0300, Anton Vasilyev wrote:
> > static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
> > in vpd_probe() based on header's ro and rw sizes.
> > In vpd_remove() vpd_section_destroy() performs deinitialization based
> > on enabled flag, which is set to true by vpd_sections_init().
> > This leads to call of vpd_section_destroy() on already destroyed section
> > for probe-release-probe-release sequence if first probe performs
> > ro_vpd initialization and second probe does not initialize it.
> >
>
> I am not sure if the situation described can be seen in the first place.
> The second probe would only not perform ro_vpd initialization if it fails
> prior to that, ie if it fails to allocate memory or if there is a
> consistency problem. In that case the remove function would not be called.
>
> However, there is a problem in the code: A partially failed probe will
> leave the system in inconsistent state. Example: ro section initializes,
> rw section fails to initialize. The probe will fail, but the ro section
> will not be destroyed, its sysfs attributes still exist, and its memory
> is still mapped. It would make more sense to fix _that_ problem.
> Essentially, vpd_sections_init() should clean up after itself after it
> fails to initialize a section.
>
> Note that I am not convinced that the "enabled" flag is needed in the first
> place. It is only relevant if vpd_section_destroy() is called, which only
> happens from the remove function. The remove function is only called if the
> probe function succeeded. In that case it is always set for both sections.
The problem will happen if coreboot memory changes between 2 probes so
that header.ro_size is not 0 on the first pass and is 0 on the second
pass. Not quite likely to ever happen in real life, but resetting a flag
is pretty cheap to not do it.
Thanks.
--
Dmitry
On Mon, Jul 23, 2018 at 10:23:05AM -0700, Dmitry Torokhov wrote:
> On Mon, Jul 23, 2018 at 10:13:36AM -0700, Guenter Roeck wrote:
> > On Mon, Jul 23, 2018 at 07:48:57PM +0300, Anton Vasilyev wrote:
> > > static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
> > > in vpd_probe() based on header's ro and rw sizes.
> > > In vpd_remove() vpd_section_destroy() performs deinitialization based
> > > on enabled flag, which is set to true by vpd_sections_init().
> > > This leads to call of vpd_section_destroy() on already destroyed section
> > > for probe-release-probe-release sequence if first probe performs
> > > ro_vpd initialization and second probe does not initialize it.
> > >
> >
> > I am not sure if the situation described can be seen in the first place.
> > The second probe would only not perform ro_vpd initialization if it fails
> > prior to that, ie if it fails to allocate memory or if there is a
> > consistency problem. In that case the remove function would not be called.
> >
> > However, there is a problem in the code: A partially failed probe will
> > leave the system in inconsistent state. Example: ro section initializes,
> > rw section fails to initialize. The probe will fail, but the ro section
> > will not be destroyed, its sysfs attributes still exist, and its memory
> > is still mapped. It would make more sense to fix _that_ problem.
> > Essentially, vpd_sections_init() should clean up after itself after it
> > fails to initialize a section.
> >
> > Note that I am not convinced that the "enabled" flag is needed in the first
> > place. It is only relevant if vpd_section_destroy() is called, which only
> > happens from the remove function. The remove function is only called if the
> > probe function succeeded. In that case it is always set for both sections.
>
> The problem will happen if coreboot memory changes between 2 probes so
> that header.ro_size is not 0 on the first pass and is 0 on the second
> pass. Not quite likely to ever happen in real life, but resetting a flag
> is pretty cheap to not do it.
>
If that can happen between probes, meaning it is not guaranteed to be
constant during the lifetime of the system, doesn't that mean it can
happen anytime ?
Guenter
On Mon, Jul 23, 2018 at 11:27:10AM -0700, Guenter Roeck wrote:
> On Mon, Jul 23, 2018 at 10:23:05AM -0700, Dmitry Torokhov wrote:
> > On Mon, Jul 23, 2018 at 10:13:36AM -0700, Guenter Roeck wrote:
> > > On Mon, Jul 23, 2018 at 07:48:57PM +0300, Anton Vasilyev wrote:
> > > > static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
> > > > in vpd_probe() based on header's ro and rw sizes.
> > > > In vpd_remove() vpd_section_destroy() performs deinitialization based
> > > > on enabled flag, which is set to true by vpd_sections_init().
> > > > This leads to call of vpd_section_destroy() on already destroyed section
> > > > for probe-release-probe-release sequence if first probe performs
> > > > ro_vpd initialization and second probe does not initialize it.
> > > >
> > >
> > > I am not sure if the situation described can be seen in the first place.
> > > The second probe would only not perform ro_vpd initialization if it fails
> > > prior to that, ie if it fails to allocate memory or if there is a
> > > consistency problem. In that case the remove function would not be called.
> > >
> > > However, there is a problem in the code: A partially failed probe will
> > > leave the system in inconsistent state. Example: ro section initializes,
> > > rw section fails to initialize. The probe will fail, but the ro section
> > > will not be destroyed, its sysfs attributes still exist, and its memory
> > > is still mapped. It would make more sense to fix _that_ problem.
> > > Essentially, vpd_sections_init() should clean up after itself after it
> > > fails to initialize a section.
> > >
> > > Note that I am not convinced that the "enabled" flag is needed in the first
> > > place. It is only relevant if vpd_section_destroy() is called, which only
> > > happens from the remove function. The remove function is only called if the
> > > probe function succeeded. In that case it is always set for both sections.
> >
> > The problem will happen if coreboot memory changes between 2 probes so
> > that header.ro_size is not 0 on the first pass and is 0 on the second
> > pass. Not quite likely to ever happen in real life, but resetting a flag
> > is pretty cheap to not do it.
> >
>
> If that can happen between probes, meaning it is not guaranteed to be
> constant during the lifetime of the system, doesn't that mean it can
> happen anytime ?
I think we can assume that the data is stable while coreboot device is
registered, but I can imagine one can theoretically have a debug
coreboot data provider that can supply different coreboot parameters
across load/unload. I.e. we have coreboot_table-acpi.c and
coreboot_table-of.c, we might create coreboot_table-test.c to feed
arbitrary data to the subsystem.
Thanks.
--
Dmitry
static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
in vpd_probe() based on header's ro and rw sizes.
In vpd_remove() vpd_section_destroy() performs deinitialization based
on enabled flag, which is set to true by vpd_sections_init().
This leads to call of vpd_section_destroy() on already destroyed section
for probe-release-probe-release sequence if first probe performs
ro_vpd initialization and second probe does not initialize it.
The patch adds changing enabled flag on vpd_section_destroy and adds
cleanup on the error path of vpd_sections_init.
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Anton Vasilyev <[email protected]>
---
v2: add cleanup on the error path of vpd_sections_init
---
drivers/firmware/google/vpd.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index e9db895916c3..1aa67bb5d8c0 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -246,6 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec)
sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr);
kfree(sec->raw_name);
memunmap(sec->baseaddr);
+ sec->enabled = false;
}
return 0;
@@ -279,8 +280,10 @@ static int vpd_sections_init(phys_addr_t physaddr)
ret = vpd_section_init("rw", &rw_vpd,
physaddr + sizeof(struct vpd_cbmem) +
header.ro_size, header.rw_size);
- if (ret)
+ if (ret) {
+ vpd_section_destroy(&ro_vpd);
return ret;
+ }
}
return 0;
--
2.18.0
On Tue, Jul 24, 2018 at 06:10:38PM +0300, Anton Vasilyev wrote:
> static struct ro_vpd and rw_vpd are initialized by vpd_sections_init()
> in vpd_probe() based on header's ro and rw sizes.
> In vpd_remove() vpd_section_destroy() performs deinitialization based
> on enabled flag, which is set to true by vpd_sections_init().
> This leads to call of vpd_section_destroy() on already destroyed section
> for probe-release-probe-release sequence if first probe performs
> ro_vpd initialization and second probe does not initialize it.
>
> The patch adds changing enabled flag on vpd_section_destroy and adds
> cleanup on the error path of vpd_sections_init.
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Anton Vasilyev <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
> ---
> v2: add cleanup on the error path of vpd_sections_init
> ---
> drivers/firmware/google/vpd.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> index e9db895916c3..1aa67bb5d8c0 100644
> --- a/drivers/firmware/google/vpd.c
> +++ b/drivers/firmware/google/vpd.c
> @@ -246,6 +246,7 @@ static int vpd_section_destroy(struct vpd_section *sec)
> sysfs_remove_bin_file(vpd_kobj, &sec->bin_attr);
> kfree(sec->raw_name);
> memunmap(sec->baseaddr);
> + sec->enabled = false;
> }
>
> return 0;
> @@ -279,8 +280,10 @@ static int vpd_sections_init(phys_addr_t physaddr)
> ret = vpd_section_init("rw", &rw_vpd,
> physaddr + sizeof(struct vpd_cbmem) +
> header.ro_size, header.rw_size);
> - if (ret)
> + if (ret) {
> + vpd_section_destroy(&ro_vpd);
> return ret;
> + }
> }
>
> return 0;
> --
> 2.18.0
>