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

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

[Marc Kleine-Budde]

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

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);
-- 
2.33.0

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

[Ziyang Xuan (William)]

> 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);
> 

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

[Xiaochen Zou]

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);
>>