Re: [PATCH 1/1] PM: fix oops in suspend/hibernate code

Jiri Slaby
2011-01-06T09:31:47Z

On 01/06/2011 10:18 AM, Jiri Slaby wrote:
> When ioremap fails (which might happen for some reason), we nicely
> oops in suspend_nvs_save due to NULL dereference by memcpy in there.
> Fail gracefully instead.
>
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> ---

Just to note, I'm not 100% sure whether the fail paths are OK with this
patch. But it works for me.

So it needs an ACK from Rafael or whoever wrote/understand the code.

> drivers/acpi/sleep.c | 5 ++---
> include/linux/suspend.h | 4 ++--
> kernel/power/nvs.c | 8 +++++++-
> 3 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index c423231..f94c9a9 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -124,8 +124,7 @@ static int acpi_pm_freeze(void)
> static int acpi_pm_pre_suspend(void)
> {
> acpi_pm_freeze();
> - suspend_nvs_save();
> - return 0;
> + return suspend_nvs_save();
> }
>
> /**
> @@ -151,7 +150,7 @@ static int acpi_pm_prepare(void)
> {
> int error = __acpi_pm_prepare();
> if (!error)
> - acpi_pm_pre_suspend();
> + error = acpi_pm_pre_suspend();
>
> return error;
> }
> diff --git a/include/linux/suspend.h b/include/linux/suspend.h
> index c1f4998..3ac2551 100644
> --- a/include/linux/suspend.h
> +++ b/include/linux/suspend.h
> @@ -262,7 +262,7 @@ static inline bool system_entering_hibernation(void) { return false; }
> extern int suspend_nvs_register(unsigned long start, unsigned long size);
> extern int suspend_nvs_alloc(void);
> extern void suspend_nvs_free(void);
> -extern void suspend_nvs_save(void);
> +extern int suspend_nvs_save(void);
> extern void suspend_nvs_restore(void);
> #else /* CONFIG_SUSPEND_NVS */
> static inline int suspend_nvs_register(unsigned long a, unsigned long b)
> @@ -271,7 +271,7 @@ static inline int suspend_nvs_register(unsigned long a, unsigned long b)
> }
> static inline int suspend_nvs_alloc(void) { return 0; }
> static inline void suspend_nvs_free(void) {}
> -static inline void suspend_nvs_save(void) {}
> +static inline int suspend_nvs_save(void) {}
> static inline void suspend_nvs_restore(void) {}
> #endif /* CONFIG_SUSPEND_NVS */
>
> diff --git a/kernel/power/nvs.c b/kernel/power/nvs.c
> index 1836db6..57c6fab 100644
> --- a/kernel/power/nvs.c
> +++ b/kernel/power/nvs.c
> @@ -105,7 +105,7 @@ int suspend_nvs_alloc(void)
> /**
> * suspend_nvs_save - save NVS memory regions
> */
> -void suspend_nvs_save(void)
> +int suspend_nvs_save(void)
> {
> struct nvs_page *entry;
>
> @@ -114,8 +114,14 @@ void suspend_nvs_save(void)
> list_for_each_entry(entry, &nvs_list, node)
> if (entry->data) {
> entry->kaddr = ioremap(entry->phys_start, entry->size);
> + if (!entry->kaddr) {
> + suspend_nvs_free();
> + return -ENOMEM;
> + }
> memcpy(entry->data, entry->kaddr, entry->size);
> }
> +
> + return 0;
> }
>
> /**

--
js