linux-kernel - Re: [Patch] Zarlink zl10036 DVB-S: Fix mem leak in zl10036

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:	Thu, 17 Feb 2011 20:54:06 +0100
From:	Matthias Schwarzott <zzam@...too.org>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	Dan Carpenter <error27@...il.com>, Tejun Heo <tj@...nel.org>,
	Mauro Carvalho Chehab <mchehab@...radead.org>
Subject: Re: [Patch] Zarlink zl10036 DVB-S: Fix mem leak in zl10036_attach

On Sunday 06 February 2011, Jesper Juhl wrote:
> If the memory allocation to 'state' succeeds but we jump to the 'error'
> label before 'state' is assigned to fe->tuner_priv, then the call to
> 'zl10036_release(fe)' at the 'error:' label will not free 'state', but
> only what was previously assigned to 'tuner_priv', thus leaking the memory
> allocated to 'state'.
> There are may ways to fix this, including assigning the allocated memory
> directly to 'fe->tuner_priv', but I did not go for that since the
> additional pointer derefs are more expensive than the local variable, so I
> just added a 'kfree(state)' call. I guess the call to 'zl10036_release'
> might not even be needed in this case, but I wasn't sure, so I left it in.
> 
Yeah, that call to zl10036_release can be completely eleminated.
Another thing is: jumping to the error label only makes sense when memory was 
already allocated. So the jump in line 471 can be replaced by "return NULL", 
as the other error handling before allocation:
        if (NULL == config) {
                printk(KERN_ERR "%s: no config specified", __func__);
                goto error;
        }

I suggest to improve the patch to clean the code up when changing that.

But I am fine with commiting this patch also if you do not want to change it.

Regards
Matthias

Signed-off-by: Matthias Schwarzott <zzam@...too.org>

> Signed-off-by: Jesper Juhl <jj@...osbits.net>
> ---
>  zl10036.c |    1 +
>  1 file changed, 1 insertion(+)
> 
>  compile tested only.
> 
> diff --git a/drivers/media/dvb/frontends/zl10036.c
> b/drivers/media/dvb/frontends/zl10036.c index 4627f49..b4fb8e8 100644
> --- a/drivers/media/dvb/frontends/zl10036.c
> +++ b/drivers/media/dvb/frontends/zl10036.c
> @@ -508,6 +508,7 @@ struct dvb_frontend *zl10036_attach(struct dvb_frontend
> *fe,
> 
>  error:
>  	zl10036_release(fe);
> +	kfree(state);
>  	return NULL;
>  }
>  EXPORT_SYMBOL(zl10036_attach);

--
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/