Re: cleanup, fix for potential crash of hotkey.c

[Rolf Eike Beer <eike-kernel@sf-tec.de>]

Handle X wrote:
> Hi,
> While going through the code, I found out some memory leaks and
> potential crashes in drivers/acpi/hotkey.c Please find the patch to
> fix them.

> Please let me know your comments.

>  static char *format_result(union acpi_object *object)
>  {
> -	char *buf = NULL;
> -
> -	buf = (char *)kmalloc(RESULT_STR_LEN, GFP_KERNEL);
> -	if (buf)
> -		memset(buf, 0, RESULT_STR_LEN);
> -	else
> -		goto do_fail;
> +	char *buf;
>
> +	buf = (char *)kzalloc(RESULT_STR_LEN, GFP_KERNEL);

no cast here

> @@ -486,98 +490,106 @@ static void free_hotkey_device(union acp
>
>  static void free_hotkey_buffer(union acpi_hotkey *key)
>  {
> -	kfree(key->event_hotkey.action_method);
> +	if (key && key->event_hotkey.action_method)
> +		kfree(key->event_hotkey.action_method);
>  }
>
>  static void free_poll_hotkey_buffer(union acpi_hotkey *key)
>  {
> -	kfree(key->poll_hotkey.action_method);
> -	kfree(key->poll_hotkey.poll_method);
> -	kfree(key->poll_hotkey.poll_result);
> +	if (!key)
> +		return;
> +	if (key->poll_hotkey.action_method)
> +		kfree(key->poll_hotkey.action_method);
> +	if (key->poll_hotkey.poll_method)
> +		kfree(key->poll_hotkey.poll_method);
> +	if (key->poll_hotkey.poll_result)
> +		kfree(key->poll_hotkey.poll_result);
>  }

No, this is the wrong way around. kfree() works fine on NULL, it just returns. 
While your change to check if key is valid makes sense the rest does not.

Anyway it's possibly better to not check for key anyway. I don't know the ACPI 
code, so things may be a bit different, but in PCI Hotplug code we removed 
many of this checks completely. These functions might not be called with a 
NULL pointer anyway, so we'll get a nice bug if someone is doing it. This way 
the bug is hidden and might lead to more obscure behaviour.

> +	for (i = 0; i < LAST_CONF_ENTRY; i++) {
> +		tmp1 = strchr(tmp, ':');
> +		if (!tmp1) {
> +			goto do_fail;
> +		}
> +		count = tmp1 - tmp;
> +		config_entry[i]= (char *)kzalloc(count + 1, GFP_KERNEL);

again no cast here

> +		if (!config_entry[i])
> +			goto handle_failure;
> +		strncpy(config_entry[i], tmp, count);
> +		tmp = tmp1 + 1;
> +	}
> +	if (sscanf(tmp, "%d:%d", internal_event_num, external_event_num) <= 0)
> +		goto handle_failure;
> +	if (!IS_OTHERS(*internal_event_num)) {
> +		return 6;
> +	}
> +handle_failure:
> +	while (i-- > 0) {
> +		kfree (config_entry[i]);
> +	}

braces unneeded, please remove the space after kfree

> @@ -736,50 +718,33 @@ static ssize_t hotkey_write_config(struc
>  				   size_t count, loff_t * data)
>  {
>  	char *config_record = NULL;
> -	char *bus_handle = NULL;
> -	char *bus_method = NULL;
> -	char *action_handle = NULL;
> -	char *method = NULL;
> +	char *config_entry[LAST_CONF_ENTRY];
>  	int cmd, internal_event_num, external_event_num;
>  	int ret = 0;
> -	union acpi_hotkey *key = NULL;
> -
> +	union acpi_hotkey *key = kzalloc(sizeof(union acpi_hotkey), GFP_KERNEL);
>
> -	config_record = (char *)kmalloc(count + 1, GFP_KERNEL);
> +	if (!key) {
> +		return -ENOMEM;
> +	}
> +	config_record = (char *)kzalloc(count + 1, GFP_KERNEL);

again no cast

>  	if (!config_record)
> +		kfree (key);

space

>  		return -ENOMEM;
>
>  	if (copy_from_user(config_record, buffer, count)) {
>  		kfree(config_record);
> +		kfree (key);

space

> @@ -923,10 +885,9 @@ static ssize_t hotkey_execute_aml_method
>  	union acpi_hotkey *key;
>
>
> -	arg = (char *)kmalloc(count + 1, GFP_KERNEL);
> +	arg = (char *)kzalloc(count + 1, GFP_KERNEL);

cast

Eike

Attachment: pgpjaPN6CbNAM.pgp
Description: PGP signature