* [PATCH] can: j1939: j1939_session_deactivate(): fix potential UAF access
@ 2021-10-17 12:01 Marc Kleine-Budde
2021-10-18 3:06 ` Ziyang Xuan (William)
0 siblings, 1 reply; 3+ messages in thread
From: Marc Kleine-Budde @ 2021-10-17 12:01 UTC (permalink / raw)
To: linux-can; +Cc: Xiaochen Zou, Ziyang Xuan, Oleksij Rempel, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] can: j1939: j1939_session_deactivate(): fix potential UAF access
2021-10-17 12:01 [PATCH] can: j1939: j1939_session_deactivate(): fix potential UAF access Marc Kleine-Budde
@ 2021-10-18 3:06 ` Ziyang Xuan (William)
2021-10-18 5:27 ` Xiaochen Zou
0 siblings, 1 reply; 3+ messages in thread
From: Ziyang Xuan (William) @ 2021-10-18 3:06 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: Xiaochen Zou, Oleksij Rempel
> 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);
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] can: j1939: j1939_session_deactivate(): fix potential UAF access
2021-10-18 3:06 ` Ziyang Xuan (William)
@ 2021-10-18 5:27 ` Xiaochen Zou
0 siblings, 0 replies; 3+ messages in thread
From: Xiaochen Zou @ 2021-10-18 5:27 UTC (permalink / raw)
To: Ziyang Xuan (William), Marc Kleine-Budde, linux-can; +Cc: Oleksij Rempel
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);
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-18 5:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-17 12:01 [PATCH] can: j1939: j1939_session_deactivate(): fix potential UAF access Marc Kleine-Budde
2021-10-18 3:06 ` Ziyang Xuan (William)
2021-10-18 5:27 ` Xiaochen Zou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).