linux-kernel - Re: [PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER() macro to avoid possible memory leak

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Mon, 17 Aug 2015 20:31:52 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Raphaël Beamonte <raphael.beamonte@...il.com>
Cc:	Johnny Kim <johnny.kim@...el.com>,
	Rachel Kim <rachel.kim@...el.com>,
	Dean Lee <dean.lee@...el.com>,
	Chris Park <chris.park@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-wireless@...r.kernel.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] staging: wilc1000: replace MALLOC_WILC_BUFFER()
 macro to avoid possible memory leak

On Mon, Aug 17, 2015 at 12:08:35PM -0400, Raphaël Beamonte wrote:
> The MACRO_WILC_BUFFER() macro was using a return statement, and didn't
> take care of possible memory leaks and subsequent bugs when it was failing
> after succeeding some allocations. This patch corrects this behavior.
> 
> Signed-off-by: Raphaël Beamonte <raphael.beamonte@...il.com>
> ---
>  drivers/staging/wilc1000/wilc_exported_buf.c | 39 +++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/wilc1000/wilc_exported_buf.c b/drivers/staging/wilc1000/wilc_exported_buf.c
> index bf392fb..c3aff9a 100644
> --- a/drivers/staging/wilc1000/wilc_exported_buf.c
> +++ b/drivers/staging/wilc1000/wilc_exported_buf.c
> @@ -8,13 +8,6 @@
>  #define LINUX_TX_SIZE	(64 * 1024)
>  #define WILC1000_FW_SIZE (4 * 1024)
>  
> -#define MALLOC_WILC_BUFFER(name, size)	\
> -	exported_ ## name = kmalloc(size, GFP_KERNEL);	  \
> -	if (!exported_ ## name) {   \
> -		printk("fail to alloc: %s memory\n", exported_ ## name);  \
> -		return -ENOBUFS;	\
> -	}
> -
>  /*
>   * Add necessary buffer pointers
>   */
> @@ -40,17 +33,43 @@ void *get_fw_buffer(void)
>  }
>  EXPORT_SYMBOL(get_fw_buffer);
>  
> +static inline int kmalloc_wilc_buffer(void *buf, int size)
> +{
> +	buf = kmalloc(size, GFP_KERNEL);
> +	if (!buf) {
> +		printk("fail to alloc memory\n");
> +		return -ENOBUFS;
> +	}
> +	return 0;
> +}
> +
>  static int __init wilc_module_init(void)
>  {
>  	printk("wilc_module_init\n");
>  	/*
>  	 * alloc necessary memory
>  	 */
> -	MALLOC_WILC_BUFFER(g_tx_buf, LINUX_TX_SIZE)
> -	MALLOC_WILC_BUFFER(g_rx_buf, LINUX_RX_SIZE)
> -	MALLOC_WILC_BUFFER(g_fw_buf, WILC1000_FW_SIZE)
> +	if (kmalloc_wilc_buffer(exported_g_tx_buf, LINUX_TX_SIZE))
> +		goto error_g_tx_buf;

Don't do it this way.  Just do:

	exported_g_tx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
	if (!exported_g_tx_buf)
		return -ENOMEM;

	exported_g_rx_buf = kmalloc(LINUX_TX_SIZE, GFP_KERNEL);
	if (!exported_g_rx_buf)
		goto free_tx_buf;

1) Avoid abstraction where possible.
2) The error code should be -ENOMEM
3) This is not a universal rule, but I feel it's better to return
   directly instead of using a do-nothing-goto.  Some people think
   using gotos everywhere forces you to think about error handling so
   it is worth making the code slightly less readable.  Based on my
   static analysis results, I do not believe that it makes people think
   about error handling.
4) Name the label after the where the label is (what it does on the next
   line) instead of basing it on the goto location.
5) Preserve the error codes.  Use ret = kmalloc_wilc_buffer(); if (ret).

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/