linux-kernel - Re: [PATCH] tm6000: Don't use pointer after freeing it in tm6000_ir

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]

Message-ID: <20120206070939.GA19754@avionic-0098.mockup.avionic-design.de>
Date:	Mon, 6 Feb 2012 08:09:39 +0100
From:	Thierry Reding <thierry.reding@...onic-design.de>
To:	Jesper Juhl <jj@...osbits.net>
Cc:	Mauro Carvalho Chehab <mchehab@...radead.org>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Curtis McEnroe <programble@...il.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tm6000: Don't use pointer after freeing it in
 tm6000_ir_fini()

* Jesper Juhl wrote:
> In tm6000_ir_fini() there seems to be a problem. 
> rc_unregister_device(ir->rc); calls rc_free_device() on the pointer it is 
> given, which frees it.
> 
> Subsequently the function does:
> 
>   if (!ir->polling)
>     __tm6000_ir_int_stop(ir->rc);
> 
> and __tm6000_ir_int_stop() dereferences the pointer it is given, which
> has already been freed.
> 
> and it also does:
> 
>   tm6000_ir_stop(ir->rc);
> 
> which also dereferences the (already freed) pointer.
> 
> So, it seems that the call to rc_unregister_device() should be move
> below the calls to __tm6000_ir_int_stop() and tm6000_ir_stop(), so
> those don't operate on a already freed pointer.
> 
> But, I must admit that I don't know this code *at all*, so someone who
> knows the code should take a careful look before applying this
> patch. It is based purely on inspection of facts of what is beeing
> freed where and not at all on understanding what the code does or why.
> I don't even have a means to test it, so beyond testing that the
> change compiles it has seen no testing what-so-ever.
> 
> Anyway, here's a proposed patch.
> 
> Signed-off-by: Jesper Juhl <jj@...osbits.net>
> ---
>  drivers/media/video/tm6000/tm6000-input.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/tm6000/tm6000-input.c b/drivers/media/video/tm6000/tm6000-input.c
> index 7844607..859eb90 100644
> --- a/drivers/media/video/tm6000/tm6000-input.c
> +++ b/drivers/media/video/tm6000/tm6000-input.c
> @@ -481,8 +481,6 @@ int tm6000_ir_fini(struct tm6000_core *dev)
>  
>  	dprintk(2, "%s\n",__func__);
>  
> -	rc_unregister_device(ir->rc);
> -
>  	if (!ir->polling)
>  		__tm6000_ir_int_stop(ir->rc);
>  
> @@ -492,6 +490,7 @@ int tm6000_ir_fini(struct tm6000_core *dev)
>  	tm6000_flash_led(dev, 0);
>  	ir->pwled = 0;
>  
> +	rc_unregister_device(ir->rc);
>  
>  	kfree(ir);
>  	dev->ir = NULL;
> -- 
> 1.7.8.4

Reviewed-by: Thierry Reding <thierry.reding@...onic-design.de>

Content of type "application/pgp-signature" skipped