Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1417854 > unrolled thread

Re: [PATCH] intel_menlow: prevent NULL pointer dereference

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] intel_menlow: prevent NULL pointer dereference Darren Hart <dvhart@infradead.org> - 2016-06-08 22:40 +0200
    Re: [PATCH] intel_menlow: prevent NULL pointer dereference Vincent Stehlé <vincent.stehle@intel.com> - 2016-06-09 19:30 +0200
      Re: [PATCH] intel_menlow: prevent NULL pointer dereference Darren Hart <dvhart@infradead.org> - 2016-06-09 22:00 +0200

#1417854 — Re: [PATCH] intel_menlow: prevent NULL pointer dereference

On Wed, May 25, 2016 at 04:20:11PM +0200, Vincent Stehlé wrote:
> The function acpi_driver_data() will dereference its parameter; make sure
> to check for NULL pointer before we call it.

+Rafael

Under what circumstances can the .remove op be called with a NULL struct
acpi_device * as a parameter? From what I can see, most acpi_* calls accpeting
an acpi_device rely on it not being null, and they are regularly called from
driver remove functions.

Did you observe an explicit failure or can you describe a call path where this
can occur?


> 
> Signed-off-by: Vincent Stehlé <vincent.stehle@intel.com>
> Cc: Sujith Thomas <sujith.thomas@intel.com>
> Cc: Darren Hart <dvhart@infradead.org>
> Cc: Zhang Rui <rui.zhang@intel.com>
> Cc: Len Brown <len.brown@intel.com>
> ---
>  drivers/platform/x86/intel_menlow.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_menlow.c b/drivers/platform/x86/intel_menlow.c
> index 0a919d8..185a1bd 100644
> --- a/drivers/platform/x86/intel_menlow.c
> +++ b/drivers/platform/x86/intel_menlow.c
> @@ -196,9 +196,13 @@ static int intel_menlow_memory_add(struct acpi_device *device)
>  
>  static int intel_menlow_memory_remove(struct acpi_device *device)
>  {
> -	struct thermal_cooling_device *cdev = acpi_driver_data(device);
> +	struct thermal_cooling_device *cdev;
> +
> +	if (!device)
> +		return -EINVAL;
>  
> -	if (!device || !cdev)
> +	cdev = acpi_driver_data(device);
> +	if (!cdev)
>  		return -EINVAL;
>  
>  	sysfs_remove_link(&device->dev.kobj, "thermal_cooling");
> -- 
> 2.8.1
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [next] | [standalone]

#1418514

On Wed, Jun 08, 2016 at 01:38:46PM -0700, Darren Hart wrote:
> Under what circumstances can the .remove op be called with a NULL struct
> acpi_device * as a parameter? From what I can see, most acpi_* calls accpeting
> an acpi_device rely on it not being null, and they are regularly called from
> driver remove functions.
> Did you observe an explicit failure or can you describe a call path where this
> can occur?

Hi Darren,

Thank you for reviewing.

I am not sure about when the .remove() functions are called with a NULL
pointer, or if that can ever happen. I just noticed that dereferencing the
pointer and checking for NULL after did not seem to be the right thing to
do. So I wanted to replicate instead the same construct as e.g.
xen_acpi_processor_remove().

Your remark encouraged me to do some more digging into the sources and it
appears that 13 .remove() functions do indeed check their input device
pointer for NULL, while 26 do not (the remaining do not use their input
pointer at all). Now I am puzzled about the necessity to check the pointer
for NULL or not, and there does not seem to be a definitive answer in the
docs either...

Best regards,

Vincent.

[toc] | [prev] | [next] | [standalone]

#1418617

On Thu, Jun 09, 2016 at 07:24:52PM +0200, Vincent Stehlé wrote:
> On Wed, Jun 08, 2016 at 01:38:46PM -0700, Darren Hart wrote:
> > Under what circumstances can the .remove op be called with a NULL struct
> > acpi_device * as a parameter? From what I can see, most acpi_* calls accpeting
> > an acpi_device rely on it not being null, and they are regularly called from
> > driver remove functions.
> > Did you observe an explicit failure or can you describe a call path where this
> > can occur?
> 
> Hi Darren,
> 
> Thank you for reviewing.
> 
> I am not sure about when the .remove() functions are called with a NULL
> pointer, or if that can ever happen. I just noticed that dereferencing the
> pointer and checking for NULL after did not seem to be the right thing to
> do. So I wanted to replicate instead the same construct as e.g.
> xen_acpi_processor_remove().
> 
> Your remark encouraged me to do some more digging into the sources and it
> appears that 13 .remove() functions do indeed check their input device
> pointer for NULL, while 26 do not (the remaining do not use their input
> pointer at all). Now I am puzzled about the necessity to check the pointer
> for NULL or not, and there does not seem to be a definitive answer in the
> docs either...

Either way, some change appears to be needed.

Rafael, with respect to acpi .remove functions, is it even possible to be called
with a NULL struct acpi_device *?

-- 
Darren Hart
Intel Open Source Technology Center

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web