[PATCH] staging: vt6655: fix potential memory leak

[PATCH] staging: vt6655: fix potential memory leak

[Nam Cao]

In function device_init_td0_ring, memory is allocated for member
td_info of priv->apTD0Rings[i], with i increasing from 0. In case of
allocation failure, the memory is freed in reversed order, with i
decreasing to 0. However, the case i=0 is left out and thus memory is
leaked.

Modify the memory freeing loop to include the case i=0.

Signed-off-by: Nam Cao <namcaov@gmail.com>
---
 drivers/staging/vt6655/device_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 3397c78b975a..a65014195fdc 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -743,7 +743,7 @@ static int device_init_td0_ring(struct vnt_private *priv)
 	return 0;
 
 err_free_desc:
-	while (--i) {
+	while (i--) {
 		desc = &priv->apTD0Rings[i];
 		kfree(desc->td_info);
 	}
-- 
2.25.1

Re: [PATCH] staging: vt6655: fix potential memory leak

[Nam Cao]

I did not realize this initially, but this bug can cause more serious problem
than just a memory leak. In the case that kzalloc fails right from the
beginning with i=0; then in the while loop, "i" will wrap around and the code
will access priv->apTD0Rings[4294967295] which is obviously not good.

Best regards,
Nam

Re: [PATCH] staging: vt6655: fix potential memory leak

[Philipp Hortmann]

On 9/9/22 16:13, Nam Cao wrote:
> In function device_init_td0_ring, memory is allocated for member
> td_info of priv->apTD0Rings[i], with i increasing from 0. In case of
> allocation failure, the memory is freed in reversed order, with i
> decreasing to 0. However, the case i=0 is left out and thus memory is
> leaked.
> 
> Modify the memory freeing loop to include the case i=0.
> 
> Signed-off-by: Nam Cao <namcaov@gmail.com>
> ---
>   drivers/staging/vt6655/device_main.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index 3397c78b975a..a65014195fdc 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -743,7 +743,7 @@ static int device_init_td0_ring(struct vnt_private *priv)
>   	return 0;
>   
>   err_free_desc:
> -	while (--i) {
> +	while (i--) {
>   		desc = &priv->apTD0Rings[i];
>   		kfree(desc->td_info);
>   	}

Tested-by: Philipp Hortmann <philipp.g.hortmann@gmail.com>

Re: [PATCH] staging: vt6655: fix potential memory leak

[Dan Carpenter]

On Fri, Sep 09, 2022 at 04:13:39PM +0200, Nam Cao wrote:
> In function device_init_td0_ring, memory is allocated for member
> td_info of priv->apTD0Rings[i], with i increasing from 0. In case of
> allocation failure, the memory is freed in reversed order, with i
> decreasing to 0. However, the case i=0 is left out and thus memory is
> leaked.
> 
> Modify the memory freeing loop to include the case i=0.
> 
> Signed-off-by: Nam Cao <namcaov@gmail.com>

Looks good.  Please add a Fixes tag, and CC all the people who were
involved with the original patch so they can review it.

Fixes: 5341ee0adb17 ("staging: vt6655: check for memory allocation failures")

I actually wrote that buggy code and Ji-Hun copied it from me, so my
bad.  Sorry!

regards,
dan carpenter

Re: [PATCH] staging: vt6655: fix potential memory leak

[Dan Carpenter]

On Fri, Sep 09, 2022 at 04:42:05PM +0200, Nam Cao wrote:
> I did not realize this initially, but this bug can cause more serious problem
> than just a memory leak. In the case that kzalloc fails right from the
> beginning with i=0; then in the while loop, "i" will wrap around and the code
> will access priv->apTD0Rings[4294967295] which is obviously not good.
> 

True.  You probably want to resend with that added to the commit
message.  I will create a Smatch check to prevent these in the future.

regards,
dan carpenter

Re: [PATCH] staging: vt6655: fix potential memory leak

[Dan Carpenter]

[-- Attachment #1: Type: text/plain, Size: 1814 bytes --]

On Mon, Sep 12, 2022 at 05:17:28PM +0300, Dan Carpenter wrote:
> On Fri, Sep 09, 2022 at 04:42:05PM +0200, Nam Cao wrote:
> > I did not realize this initially, but this bug can cause more serious problem
> > than just a memory leak. In the case that kzalloc fails right from the
> > beginning with i=0; then in the while loop, "i" will wrap around and the code
> > will access priv->apTD0Rings[4294967295] which is obviously not good.
> > 
> 
> True.  You probably want to resend with that added to the commit
> message.  I will create a Smatch check to prevent these in the future.
> 

I created a static checker warning for these and it found a bunch more
bugs in the same file.

drivers/staging/vt6655/device_main.c:635 device_init_rd0_ring() warn: potential decrement underflow 'i' rl='s32min-(-1)' (iterator)
drivers/staging/vt6655/device_main.c:636 device_init_rd0_ring() warn: using underflowed offset 'i'
drivers/staging/vt6655/device_main.c:681 device_init_rd1_ring() warn: potential decrement underflow 'i' rl='s32min-(-1)' (iterator)
drivers/staging/vt6655/device_main.c:682 device_init_rd1_ring() warn: using underflowed offset 'i'
drivers/staging/vt6655/device_main.c:746 device_init_td0_ring() warn: potential decrement underflow 'i' rl='s32min-(-1)' (iterator)
drivers/staging/vt6655/device_main.c:747 device_init_td0_ring() warn: using underflowed offset 'i'
drivers/staging/vt6655/device_main.c:786 device_init_td1_ring() warn: potential decrement underflow 'i' rl='s32min-(-1)' (iterator)
drivers/staging/vt6655/device_main.c:787 device_init_td1_ring() warn: using underflowed offset 'i'

Could you fix those as well?  They're all the same bug and the same file
so I would do it all at once.  They were introduced in the same patch
as well so only one Fixes tag required.

regards,
dan carpenter

[-- Attachment #2: check_decrement_underflow.c --]
[-- Type: text/x-csrc, Size: 2137 bytes --]

/*
 * Copyright (C) 2022 Oracle.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version 2
 * of the License, or (at your option) any later version.
 *
 * This program is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 * GNU General Public License for more details.
 *
 * You should have received a copy of the GNU General Public License
 * along with this program; if not, see http://www.gnu.org/copyleft/gpl.txt
 */

#include "smatch.h"
#include "smatch_extra.h"
#include "smatch_slist.h"

static int my_id;

STATE(decremented);

static struct expression *prev;
static void match_condition(struct expression *expr)
{
	struct expression *orig = expr;
	struct statement *stmt;
	struct range_list *rl;
	const char *iterator = "";
	char *name;

	if (expr == prev)
		return;

	if (expr->type != EXPR_PREOP || expr->op != SPECIAL_DECREMENT)
		return;

	expr = strip_expr(expr->unop);
	get_absolute_rl(expr, &rl);
	if (rl_min(rl).value > 0)
		return;

	set_state_expr(my_id, expr, &decremented);

	stmt = get_parent_stmt(orig);
	if (stmt && stmt->type == STMT_ITERATOR)
		iterator = " (iterator)";

	prev = orig;

	name = expr_to_str(expr);
	sm_warning("potential decrement underflow '%s' rl='%s'%s", name,
		   show_rl(rl), iterator);
	free_string(name);
}

static void array_check(struct expression *expr)
{
	struct expression *array, *offset;
	struct range_list *rl;
	char *name;

	expr = strip_expr(expr);
	array = get_array_base(expr);
	offset = get_array_offset(expr);

	if (!array || !offset)
		return;

	if (!get_state_expr(my_id, offset))
		return;

	get_absolute_rl(offset, &rl);
	if (rl_min(rl).value >= 0)
		return;

	name = expr_to_str(offset);
	sm_warning("using underflowed offset '%s'", name);
	free_string(name);
}


void check_decrement_underflow(int id)
{
	my_id = id;

	add_hook(&match_condition, CONDITION_HOOK);

	add_hook(&array_check, OP_HOOK);
}