Re: [PATCH] acpi/nfit: Fix memory corruption/Unregister mce decoder on failure

From: "Verma, Vishal L" <vishal.l.verma@intel.com>
To: "prarit@redhat.com" <prarit@redhat.com>,
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
Cc: "Williams, Dan J" <dan.j.williams@intel.com>,
	"lenb@kernel.org" <lenb@kernel.org>,
	"joeyli.kernel@gmail.com" <joeyli.kernel@gmail.com>,
	"jmoyer@redhat.com" <jmoyer@redhat.com>,
	"rjw@rjwysocki.net" <rjw@rjwysocki.net>,
	"linda.knippers@hpe.com" <linda.knippers@hpe.com>
Subject: Re: [PATCH] acpi/nfit: Fix memory corruption/Unregister mce decoder on failure
Date: Thu, 18 May 2017 17:35:06 +0000	[thread overview]
Message-ID: <1495128822.20330.18.camel@intel.com> (raw)
In-Reply-To: <1495065367-30551-1-git-send-email-prarit@redhat.com>

On Wed, 2017-05-17 at 19:56 -0400, Prarit Bhargava wrote:
> nfit_init() calls nfit_mce_register() on module load.  When the module
> load fails the nfit mce decoder is not unregistered.  The module's
> memory is freed leaving the decoder chain referencing junk.  This will
> cause panics as future registrations will reference the free'd memory.
> 
> Only register the nfit mce decoder on success.

Good find! I'm wondering if registering the mce handler after the rest
of the init will leave a (small) window open where MCEs can happen but
the handler has yet to be registered.. Maybe it is inconsequential, and
we can just do this. Alternatively, if acpi_bus_register_driver fails,
then go back and unregister the mce handler..

> 
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: Jeff Moyer <jmoyer@redhat.com>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Vishal Verma <vishal.l.verma@intel.com>
> Cc: "Lee, Chun-Yi" <joeyli.kernel@gmail.com>
> Cc: Linda Knippers <linda.knippers@hpe.com>
> ---
>  drivers/acpi/nfit/core.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 656acb5d7166..b60856b4325b 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -3043,6 +3043,8 @@ static void acpi_nfit_notify(struct acpi_device
> *adev, u32 event)
>  
>  static __init int nfit_init(void)
>  {
> +	int ret;
> +
>  	BUILD_BUG_ON(sizeof(struct acpi_table_nfit) != 40);
>  	BUILD_BUG_ON(sizeof(struct acpi_nfit_system_address) != 56);
>  	BUILD_BUG_ON(sizeof(struct acpi_nfit_memory_map) != 48);
> @@ -3069,9 +3071,11 @@ static __init int nfit_init(void)
>  	if (!nfit_wq)
>  		return -ENOMEM;
>  
> -	nfit_mce_register();
> +	ret = acpi_bus_register_driver(&acpi_nfit_driver);
> +	if (!ret)
> +		nfit_mce_register();
>  
> -	return acpi_bus_register_driver(&acpi_nfit_driver);
> +	return ret;
>  }
>  
>  static __exit void nfit_exit(void)