[PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

[PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

[Jesper Juhl]

If calling put_ir_rx(rx, true); in
drivers/staging/lirc/lirc_zilog.c::ir_probe() returns true (1) then it
means that it has freed it's first argument. Subsequently jumping to
'out_put_xx' will cause us to call put_ir_rx() once more since 'rx' is
not zero - leading to a double free.
To fix this, test the return value of 'put_ir_rx()' and if it is true
(1), zero 'rx' so that we won't do it again.

I'm not familiar with this code, so this may not be the best fix, and
I also don't have the hardware to test it properly, so this patch is
compile tested only and someone who knows this code should probably
ACK it before it's merged.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 drivers/staging/lirc/lirc_zilog.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 0302d82..51ded22 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -1577,7 +1577,8 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			put_ir_device(ir, true);
 			/* Failure exit, so put back rx ref from i2c_client */
 			i2c_set_clientdata(client, NULL);
-			put_ir_rx(rx, true);
+			if (put_ir_rx(rx, true))
+				rx = NULL;
 			ir->l.features &= ~LIRC_CAN_REC_LIRCCODE;
 			goto out_put_xx;
 		}
-- 
1.7.6


-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

[Dan Carpenter]

On Thu, Jul 28, 2011 at 11:49:51PM +0200, Jesper Juhl wrote:
> If calling put_ir_rx(rx, true); in
> drivers/staging/lirc/lirc_zilog.c::ir_probe() returns true (1) then it
> means that it has freed it's first argument. Subsequently jumping to
> 'out_put_xx' will cause us to call put_ir_rx() once more since 'rx' is
> not zero - leading to a double free.

It would be better to just remove the first call to put_ir_rx().

regards,
dan carpenter

Re: [PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

[Jesper Juhl]

On Fri, 29 Jul 2011, Dan Carpenter wrote:

> On Thu, Jul 28, 2011 at 11:49:51PM +0200, Jesper Juhl wrote:
> > If calling put_ir_rx(rx, true); in
> > drivers/staging/lirc/lirc_zilog.c::ir_probe() returns true (1) then it
> > means that it has freed it's first argument. Subsequently jumping to
> > 'out_put_xx' will cause us to call put_ir_rx() once more since 'rx' is
> > not zero - leading to a double free.
> 
> It would be better to just remove the first call to put_ir_rx().
> 

Taking a second look... Yes, you are right, that would be just fine and a 
lot simpler. Thanks for reviewing.

Here's an updated patch.


From: Jesper Juhl <jj@chaosbits.net>
Subject: [PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

If the call to put_ir_rx(rx, true); in
drivers/staging/lirc/lirc_zilog.c::ir_probe() returns true (1) then it
means that it has freed it's first argument. Subsequently jumping to
'out_put_xx' will cause us to call put_ir_rx() once more since 'rx' is
not zero - leading to a double free.
To fix this, simply remove the first call to put_ir_rx().

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
 drivers/staging/lirc/lirc_zilog.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/lirc/lirc_zilog.c b/drivers/staging/lirc/lirc_zilog.c
index 0302d82..6be6f67 100644
--- a/drivers/staging/lirc/lirc_zilog.c
+++ b/drivers/staging/lirc/lirc_zilog.c
@@ -1577,7 +1577,6 @@ static int ir_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			put_ir_device(ir, true);
 			/* Failure exit, so put back rx ref from i2c_client */
 			i2c_set_clientdata(client, NULL);
-			put_ir_rx(rx, true);
 			ir->l.features &= ~LIRC_CAN_REC_LIRCCODE;
 			goto out_put_xx;
 		}
-- 
1.7.6

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

[Andy Walls]

Dan Carpenter <error27@gmail.com> wrote:

>On Thu, Jul 28, 2011 at 11:49:51PM +0200, Jesper Juhl wrote:
>> If calling put_ir_rx(rx, true); in
>> drivers/staging/lirc/lirc_zilog.c::ir_probe() returns true (1) then
>it
>> means that it has freed it's first argument. Subsequently jumping to
>> 'out_put_xx' will cause us to call put_ir_rx() once more since 'rx'
>is
>> not zero - leading to a double free.
>
>It would be better to just remove the first call to put_ir_rx().
>
>regards,
>dan carpenter

Jesper,

Could you forward your original patch email to me?  I never got it.

I was the one who added all the new ref counting to lirc_zilog and it was not fun to get right (well at least what I thought was right...).

Regards,
Andy

Re: [PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

[Jesper Juhl]

On Fri, 29 Jul 2011, Andy Walls wrote:

> Dan Carpenter <error27@gmail.com> wrote:
> 
> >On Thu, Jul 28, 2011 at 11:49:51PM +0200, Jesper Juhl wrote:
> >> If calling put_ir_rx(rx, true); in
> >> drivers/staging/lirc/lirc_zilog.c::ir_probe() returns true (1) then
> >it
> >> means that it has freed it's first argument. Subsequently jumping to
> >> 'out_put_xx' will cause us to call put_ir_rx() once more since 'rx'
> >is
> >> not zero - leading to a double free.
> >
> >It would be better to just remove the first call to put_ir_rx().
> >
> >regards,
> >dan carpenter
> 
> Jesper,
> 
> Could you forward your original patch email to me?  I never got it.
> 
> I was the one who added all the new ref counting to lirc_zilog and it was not fun to get right (well at least what I thought was right...).
> 

Sure. I've just forwarded both the original patch mail and the mail with 
the updated version to  Andy Walls <awalls@md.metrocast.net>

-- 
Jesper Juhl <jj@chaosbits.net>       http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

Re: [PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

[Andy Walls]

On Fri, 2011-07-29 at 09:08 +0300, Dan Carpenter wrote:
> On Thu, Jul 28, 2011 at 11:49:51PM +0200, Jesper Juhl wrote:
> > If calling put_ir_rx(rx, true); in
> > drivers/staging/lirc/lirc_zilog.c::ir_probe() returns true (1) then it
> > means that it has freed it's first argument. Subsequently jumping to
> > 'out_put_xx' will cause us to call put_ir_rx() once more since 'rx' is
> > not zero - leading to a double free.
> 
> It would be better to just remove the first call to put_ir_rx().

Jesper,

(Emails from you don't seem to make it to me, so I looked at your patch
in lkml.org archive.)

Good catch!

Although either fix will work, I do prefer Dan's suggested fix.  Could
you please implement that?

Since emails from you don't seem to make it to me, and since Dan's
suggestion is trivial to implement, I'll just ack that form of the fix
right now:

Acked-by: Andy Walls <awalls@md.metrocast.net>

Regards,
Andy

Re: [PATCH] staging; lirc, zilog: put_ir_rx may free 'rx' which can lead to double free

[Andy Walls]

On Sun, 2011-07-31 at 08:42 -0400, Andy Walls wrote:
> On Fri, 2011-07-29 at 09:08 +0300, Dan Carpenter wrote:
> > On Thu, Jul 28, 2011 at 11:49:51PM +0200, Jesper Juhl wrote:
> > > If calling put_ir_rx(rx, true); in
> > > drivers/staging/lirc/lirc_zilog.c::ir_probe() returns true (1) then it
> > > means that it has freed it's first argument. Subsequently jumping to
> > > 'out_put_xx' will cause us to call put_ir_rx() once more since 'rx' is
> > > not zero - leading to a double free.
> > 
> > It would be better to just remove the first call to put_ir_rx().
> 
> Jesper,
> 
> (Emails from you don't seem to make it to me, so I looked at your patch
> in lkml.org archive.)
> 
> Good catch!

Jesper,

Ah crud.  I take that back.

NACK on either form of the patch.

lirc_zilog is a ref counting PITA.  Look at ir_probe() again.

We grab 2 rx refs.

One at the line 

	kref_init(&rx->ref);

and another at

	/* An rx ref goes to the i2c_client */
        i2c_set_clientdata(client, get_ir_rx(ir));


On failure, we need two "put_ir_rx(rx, true)" calls.

That's why I wrote the comment to myself:

         /* Failure exit, so put back rx ref from i2c_client */
         i2c_set_clientdata(client, NULL);
         put_ir_rx(rx, true);

Regards,
Andy