[PATCH 4.4-stable] slip: stop double free sl->dev in slip_open

[PATCH 4.4-stable] slip: stop double free sl->dev in slip_open

[yangerkun]

After commit e4c157955483 ("slip: Fix use-after-free Read in slip_open"),
we will double free sl->dev since sl_free_netdev will free sl->dev too.
It's fine for mainline since sl_free_netdev in mainline won't free
sl->dev.

Signed-off-by: yangerkun <yangerkun@huawei.com>
---
 drivers/net/slip/slip.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
index ef6b25ec75a1..7fe9183fad0e 100644
--- a/drivers/net/slip/slip.c
+++ b/drivers/net/slip/slip.c
@@ -861,7 +861,6 @@ err_free_chan:
 	tty->disc_data = NULL;
 	clear_bit(SLF_INUSE, &sl->flags);
 	sl_free_netdev(sl->dev);
-	free_netdev(sl->dev);
 
 err_exit:
 	rtnl_unlock();
-- 
2.17.2

Re: [PATCH 4.4-stable] slip: stop double free sl->dev in slip_open

[yangerkun]

cc David and netdev mail list too.

On 2020/2/22 17:46, yangerkun wrote:
> After commit e4c157955483 ("slip: Fix use-after-free Read in slip_open"),
> we will double free sl->dev since sl_free_netdev will free sl->dev too.
> It's fine for mainline since sl_free_netdev in mainline won't free
> sl->dev.
> 
> Signed-off-by: yangerkun <yangerkun@huawei.com>
> ---
>   drivers/net/slip/slip.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index ef6b25ec75a1..7fe9183fad0e 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -861,7 +861,6 @@ err_free_chan:
>   	tty->disc_data = NULL;
>   	clear_bit(SLF_INUSE, &sl->flags);
>   	sl_free_netdev(sl->dev);
> -	free_netdev(sl->dev);
>   
>   err_exit:
>   	rtnl_unlock();
> 

Re: [PATCH 4.4-stable] slip: stop double free sl->dev in slip_open

[Greg KH]

On Mon, Feb 24, 2020 at 11:06:48AM +0800, yangerkun wrote:
> cc David and netdev mail list too.
> 
> On 2020/2/22 17:46, yangerkun wrote:
> > After commit e4c157955483 ("slip: Fix use-after-free Read in slip_open"),
> > we will double free sl->dev since sl_free_netdev will free sl->dev too.
> > It's fine for mainline since sl_free_netdev in mainline won't free
> > sl->dev.
> > 
> > Signed-off-by: yangerkun <yangerkun@huawei.com>
> > ---
> >   drivers/net/slip/slip.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> > index ef6b25ec75a1..7fe9183fad0e 100644
> > --- a/drivers/net/slip/slip.c
> > +++ b/drivers/net/slip/slip.c
> > @@ -861,7 +861,6 @@ err_free_chan:
> >   	tty->disc_data = NULL;
> >   	clear_bit(SLF_INUSE, &sl->flags);
> >   	sl_free_netdev(sl->dev);
> > -	free_netdev(sl->dev);
> >   err_exit:
> >   	rtnl_unlock();
> > 
> 

What commit causes this only to be needed on the 4.4-stable tree?  Can
you please list it in the commit log so that we know this?

And this is only for 4.4.y, not 4.9.y or anything else?  Why?

thanks,

greg k-h

Re: [PATCH 4.4-stable] slip: stop double free sl->dev in slip_open

[yangerkun]

On 2020/2/27 20:49, Greg KH wrote:
> On Mon, Feb 24, 2020 at 11:06:48AM +0800, yangerkun wrote:
>> cc David and netdev mail list too.
>>
>> On 2020/2/22 17:46, yangerkun wrote:
>>> After commit e4c157955483 ("slip: Fix use-after-free Read in slip_open"),
>>> we will double free sl->dev since sl_free_netdev will free sl->dev too.
>>> It's fine for mainline since sl_free_netdev in mainline won't free
>>> sl->dev.
>>>
>>> Signed-off-by: yangerkun <yangerkun@huawei.com>
>>> ---
>>>    drivers/net/slip/slip.c | 1 -
>>>    1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
>>> index ef6b25ec75a1..7fe9183fad0e 100644
>>> --- a/drivers/net/slip/slip.c
>>> +++ b/drivers/net/slip/slip.c
>>> @@ -861,7 +861,6 @@ err_free_chan:
>>>    	tty->disc_data = NULL;
>>>    	clear_bit(SLF_INUSE, &sl->flags);
>>>    	sl_free_netdev(sl->dev);
>>> -	free_netdev(sl->dev);
>>>    err_exit:
>>>    	rtnl_unlock();
>>>
>>
> 
> What commit causes this only to be needed on the 4.4-stable tree?  Can
> you please list it in the commit log so that we know this?
> 
> And this is only for 4.4.y, not 4.9.y or anything else?  Why?
Hi,

Sorry for does not check other stable branch!

The problem exist in 4.4 stable branch because we merged 3b5a39979daf 
("slip: Fix memory leak in slip_open error path") and e58c19124189 
("slip: Fix use-after-free Read in slip_open") without the patch 
cf124db566e6 ("net: Fix inconsistent teardown and release of private 
netdev state."). And since cf124db566e6 has remove the free_netdev exist 
in sl_free_netdev, so fault branch err_free_chan in slip_open will not 
call free_netdev twice in mainline. However, 4.4 stable branch will do it.

Futhermore, since sl_free_netdev will do the all we need, so I think 
delete the free_netdev below sl_free_netdev in slip_open will be fine to 
fix the double free problem, also two problem describes by previous two 
patch.

After check for 3.16.y/4.9.y/4.14.y/4.19.y/5.4.y/5.5.y, and the result 
show as below:

3.16.y:
No double free problem since below two commit has not merged in:
e58c19124189 slip: Fix use-after-free Read in slip_open
3b5a39979daf slip: Fix memory leak in slip_open error path

4.9.y:
problem exist

4.14.y/4.19.y/5.4.y/5.5.y:
no double free problem since cf124db566e6 ("net: Fix inconsistent 
teardown and release of private netdev state.") has been included

So, 4.9.y need this patch too! I will resend the patch for 4.4.y and 
4.9.y with commit message refresh.

Thanks,
Kun.


> 
> thanks,
> 
> greg k-h
> 
> .
>