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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 2 Aug 2006 09:28:41 +0200
From:	Rolf Eike Beer <eike-kernel@...tec.de>
To:	"Handle X" <xhandle@...il.com>
Cc:	linux-kernel@...r.kernel.org, kernel-janitors@...ts.osdl.org
Subject: Re: cleanup, fix for potential crash of hotkey.c

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

Content of type "application/pgp-signature" skipped