Re: [PATCH] can: j1939: j1939_session_deactivate(): fix potential UAF access

[Xiaochen Zou <xzou017@ucr.edu>]

Hi,
I found this bug when skimming the source code, so there is no concrete
test case right now. Probably it's just like you said, the buggy
scenario does not exist in current kernel control flow. I'm not familiar
with the logic of this module thigh, but will we guarantee that future
code won't make this possible if someone didn't follow some specific
manners?

On 10/17/2021 8:06 PM, Ziyang Xuan (William) wrote:
>> From: Xiaochen Zou <xzou017@ucr.edu>
>>
>> Both session and session->priv may be freed in
>> j1939_session_deactivate_locked(). It leads to potential UAF read and
>> write in j1939_session_list_unlock(). The free chain is:
>>
>> | j1939_session_deactivate_locked() ->
>> | j1939_session_put() ->
>> | __j1939_session_release() ->
>> | j1939_session_destroy()
>>
>> To fix this bug, move the j1939_session_put() behind
>> j1939_session_deactivate_locked(), and guard it with a check of active
>> since the session would be freed only if active is true.
>>
>> Link: https://lore.kernel.org/all/CAE1SXrv3Ouwt4Y9NEWGi0WO701w1YP1ruMSxraZr4PZTGsUZgg@mail.gmail.com
>> Link: https://lore.kernel.org/all/aa64ef28-35d8-9deb-2756-8080296b7e3e@ucr.edu
>> Cc: Ziyang Xuan <william.xuanziyang@huawei.com>
>> Cc: Oleksij Rempel <o.rempel@pengutronix.de>
>> Signed-off-by: Xiaochen Zou <xzou017@ucr.edu>
>> Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
>> ---
>> Hello,
>>
>> I picked up the patch from Xiaochen Zou. I think it is the proposed
>> fix for:
>>
>> | https://syzkaller.appspot.com/bug?extid=a47537d3964ef6c874e1
>>
>>
>> It turned out that
>>
>> | 0c71437dd50d can: j1939: j1939_session_deactivate(): clarify lifetime of session object                                                                                                                                                                         
>>
>> is wrong, and should be removed, as Ziyang Xuan proposed in:
>>
>> | https://lore.kernel.org/all/20210906094200.95868-1-william.xuanziyang@huawei.com
>>
>> Ziyang Xuan, Oleksij, can you have a look at Xiaochen Zou's patch and
>> give me an Ack, then I'll take both patches upstream.
>>
>> regards,
>> Marc
> 
> I think the session kref >= 2 when it is active state. The j1939_session_put() in
> j1939_session_deactivate_locked() will not trigger __j1939_session_release() to free
> session.
> 
> 0c71437dd50d ("can: j1939: j1939_session_deactivate(): clarify lifetime of session object") just
> only partly wrong. j1939_session_deactivate() is called not only when session is active, it may be
> called when session is not active already.
> 
> And I think the 0c71437dd50d may be the real fix for:
> https://syzkaller.appspot.com/bug?extid=a47537d3964ef6c874e1
> 
> I can not find an exact scenario as Xiaochen Zou's patch mentioned. So I can not agree.
> 
> Or can you give an exact scenario @Xiaochen Zou.
> 
> Thank you!
> 
>>
>> net/can/j1939/transport.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
>> index dc3c30810833..35530b09c84f 100644
>> --- a/net/can/j1939/transport.c
>> +++ b/net/can/j1939/transport.c
>> @@ -1072,7 +1072,6 @@ static bool j1939_session_deactivate_locked(struct j1939_session *session)
>>  
>>  		list_del_init(&session->active_session_list_entry);
>>  		session->state = J1939_SESSION_DONE;
>> -		j1939_session_put(session);
>>  	}
>>  
>>  	return active;
>> @@ -1086,6 +1085,8 @@ static bool j1939_session_deactivate(struct j1939_session *session)
>>  	j1939_session_list_lock(priv);
>>  	active = j1939_session_deactivate_locked(session);
>>  	j1939_session_list_unlock(priv);
>> +	if (active)
>> +		j1939_session_put(session);
>>  
>>  	return active;
>>  }
>> @@ -2152,6 +2153,7 @@ void j1939_simple_recv(struct j1939_priv *priv, struct sk_buff *skb)
>>  int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
>>  {
>>  	struct j1939_session *session, *saved;
>> +	bool active;
>>  
>>  	netdev_dbg(priv->ndev, "%s, sk: %p\n", __func__, sk);
>>  	j1939_session_list_lock(priv);
>> @@ -2165,7 +2167,9 @@ int j1939_cancel_active_session(struct j1939_priv *priv, struct sock *sk)
>>  				j1939_session_put(session);
>>  
>>  			session->err = ESHUTDOWN;
>> -			j1939_session_deactivate_locked(session);
>> +			active = j1939_session_deactivate_locked(session);
>> +			if (active)
>> +				j1939_session_put(session);
>>  		}
>>  	}
>>  	j1939_session_list_unlock(priv);
>>