linux-kernel - Re: [PATCH v4] staging: lustre: llite: Fix variable length array warning

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:   Tue, 9 May 2017 11:31:36 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Guru Das Srinagesh <gurooodas@...il.com>
Cc:     oleg.drokin@...el.com, andreas.dilger@...el.com,
        jsimmons@...radead.org, gregkh@...uxfoundation.org,
        joe@...ches.com, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org, lustre-devel@...ts.lustre.org
Subject: Re: [PATCH v4] staging: lustre: llite: Fix variable length array
 warning

This patch introduces tons of memory leaks so we can't apply it.

Just ignore the warning.  The size is small enough that it won't
overflow the stack.  The other reason to avoid these types of
declarations is that in olden times (5 years ago at least) there was a
GCC for an arch where if you declared the variable inside a loop it
would not free the memory until after the end of the loop.

	while ((foo = frob())) {
		int whatver[x + y];
	}

The memory would keep increasing until we broke from the loop or it
overflowed the stack and crashed.

On Mon, May 08, 2017 at 11:57:16PM -0700, Guru Das Srinagesh wrote:
> Fix sparse warning "warning: Variable length array is used." by using
> kmalloc_array to allocate the required amount of memory instead and
> kfree to deallocate memory after use.
> 
> Signed-off-by: Guru Das Srinagesh <gurooodas@...il.com>
> ---
>  v4:
>    - Changed kmalloc_array flags from GFP_KERNEL to GFP_ATOMIC
> 
>  v3:
>    - Fixed checkpatch warning: Comparison to NULL could be written "!fullname"
> 
>  v2:
>    - Added missing check for NULL return value of kmalloc_array()
> 
>  drivers/staging/lustre/lustre/llite/xattr.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/llite/xattr.c b/drivers/staging/lustre/lustre/llite/xattr.c
> index 6187bff..ae2efd5 100644
> --- a/drivers/staging/lustre/lustre/llite/xattr.c
> +++ b/drivers/staging/lustre/lustre/llite/xattr.c
> @@ -86,13 +86,17 @@ ll_xattr_set_common(const struct xattr_handler *handler,
>  		    const char *name, const void *value, size_t size,
>  		    int flags)
>  {
> -	char fullname[strlen(handler->prefix) + strlen(name) + 1];
> +	int fullname_len = strlen(handler->prefix) + strlen(name) + 1;
> +	char *fullname = kmalloc_array(fullname_len, sizeof(char), GFP_ATOMIC);

Using kmalloc_array() is pointless.  Everyone knows that sizeof(char) is
1 and also that 1 * x == x.  It just makes the code more confusing and
complicated for no reason.  Don't hide the allocation this declaration
block.  It should be next to the check for NULL.  But anyway, don't
bother resending, just ignore the warning.

regards,
dan carpenter