Commit 2030043e authored by Oleksij Rempel's avatar Oleksij Rempel Committed by Marc Kleine-Budde
Browse files

can: j1939: fix Use-after-Free, hold skb ref while in use 

This patch fixes a Use-after-Free found by the syzbot.

The problem is that a skb is taken from the per-session skb queue,
without incrementing the ref count. This leads to a Use-after-Free if
the skb is taken concurrently from the session queue due to a CTS.

Fixes: 9d71dd0c ("can: add support of SAE J1939 protocol")
Link: https://lore.kernel.org/r/20210521115720.7533-1-o.rempel@pengutronix.de


Cc: Hillf Danton <hdanton@sina.com>
Cc: linux-stable <stable@vger.kernel.org>
Reported-by: default avatar <syzbot+220c1a29987a9a490903@syzkaller.appspotmail.com>
Reported-by: default avatar <syzbot+45199c1b73b4013525cf@syzkaller.appspotmail.com>
Signed-off-by: default avatarOleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: default avatarMarc Kleine-Budde <mkl@pengutronix.de>
parent a4f0377d

net/can/j1939/transport.c

+40 −14
Original line number Diff line number Diff line
@@ -330,6 +330,9 @@ static void j1939_session_skb_drop_old(struct j1939_session *session) 


	if ((do_skcb->offset + do_skb->len) < offset_start) {

		__skb_unlink(do_skb, &session->skb_queue);

		/* drop ref taken in j1939_session_skb_queue() */

		skb_unref(do_skb);



		kfree_skb(do_skb);

	}

	spin_unlock_irqrestore(&session->skb_queue.lock, flags);
@@ -349,11 +352,12 @@ void j1939_session_skb_queue(struct j1939_session *session, 


	skcb->flags |= J1939_ECU_LOCAL_SRC;



	skb_get(skb);

	skb_queue_tail(&session->skb_queue, skb);

}



static struct

sk_buff *j1939_session_skb_find_by_offset(struct j1939_session *session,

sk_buff *j1939_session_skb_get_by_offset(struct j1939_session *session,

					 unsigned int offset_start)

{

	struct j1939_priv *priv = session->priv;
@@ -371,6 +375,10 @@ sk_buff *j1939_session_skb_find_by_offset(struct j1939_session *session, 
			skb = do_skb;

		}

	}



	if (skb)

		skb_get(skb);



	spin_unlock_irqrestore(&session->skb_queue.lock, flags);



	if (!skb)
@@ -381,12 +389,12 @@ sk_buff *j1939_session_skb_find_by_offset(struct j1939_session *session, 
	return skb;

}



static struct sk_buff *j1939_session_skb_find(struct j1939_session *session)

static struct sk_buff *j1939_session_skb_get(struct j1939_session *session)

{

	unsigned int offset_start;



	offset_start = session->pkt.dpo * 7;

	return j1939_session_skb_find_by_offset(session, offset_start);

	return j1939_session_skb_get_by_offset(session, offset_start);

}



/* see if we are receiver
@@ -776,7 +784,7 @@ static int j1939_session_tx_dat(struct j1939_session *session) 
	int ret = 0;

	u8 dat[8];



	se_skb = j1939_session_skb_find_by_offset(session, session->pkt.tx * 7);

	se_skb = j1939_session_skb_get_by_offset(session, session->pkt.tx * 7);

	if (!se_skb)

		return -ENOBUFS;
@@ -801,7 +809,8 @@ static int j1939_session_tx_dat(struct j1939_session *session) 
			netdev_err_once(priv->ndev,

					"%s: 0x%p: requested data outside of queued buffer: offset %i, len %i, pkt.tx: %i\n",

					__func__, session, skcb->offset, se_skb->len , session->pkt.tx);

			return -EOVERFLOW;

			ret = -EOVERFLOW;

			goto out_free;

		}



		if (!len) {
@@ -835,6 +844,12 @@ static int j1939_session_tx_dat(struct j1939_session *session) 
	if (pkt_done)

		j1939_tp_set_rxtimeout(session, 250);



 out_free:

	if (ret)

		kfree_skb(se_skb);

	else

		consume_skb(se_skb);



	return ret;

}
@@ -1007,7 +1022,7 @@ static int j1939_xtp_txnext_receiver(struct j1939_session *session) 
static int j1939_simple_txnext(struct j1939_session *session)

{

	struct j1939_priv *priv = session->priv;

	struct sk_buff *se_skb = j1939_session_skb_find(session);

	struct sk_buff *se_skb = j1939_session_skb_get(session);

	struct sk_buff *skb;

	int ret;
@@ -1015,8 +1030,10 @@ static int j1939_simple_txnext(struct j1939_session *session) 
		return 0;



	skb = skb_clone(se_skb, GFP_ATOMIC);

	if (!skb)

		return -ENOMEM;

	if (!skb) {

		ret = -ENOMEM;

		goto out_free;

	}



	can_skb_set_owner(skb, se_skb->sk);
@@ -1024,12 +1041,18 @@ static int j1939_simple_txnext(struct j1939_session *session) 


	ret = j1939_send_one(priv, skb);

	if (ret)

		return ret;

		goto out_free;



	j1939_sk_errqueue(session, J1939_ERRQUEUE_SCHED);

	j1939_sk_queue_activate_next(session);



	return 0;

 out_free:

	if (ret)

		kfree_skb(se_skb);

	else

		consume_skb(se_skb);



	return ret;

}



static bool j1939_session_deactivate_locked(struct j1939_session *session)
@@ -1170,9 +1193,10 @@ static void j1939_session_completed(struct j1939_session *session) 
	struct sk_buff *skb;



	if (!session->transmission) {

		skb = j1939_session_skb_find(session);

		skb = j1939_session_skb_get(session);

		/* distribute among j1939 receivers */

		j1939_sk_recv(session->priv, skb);

		consume_skb(skb);

	}



	j1939_session_deactivate_activate_next(session);
@@ -1744,7 +1768,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, 
{

	struct j1939_priv *priv = session->priv;

	struct j1939_sk_buff_cb *skcb;

	struct sk_buff *se_skb;

	struct sk_buff *se_skb = NULL;

	const u8 *dat;

	u8 *tpdat;

	int offset;
@@ -1786,7 +1810,7 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, 
		goto out_session_cancel;

	}



	se_skb = j1939_session_skb_find_by_offset(session, packet * 7);

	se_skb = j1939_session_skb_get_by_offset(session, packet * 7);

	if (!se_skb) {

		netdev_warn(priv->ndev, "%s: 0x%p: no skb found\n", __func__,

			    session);
@@ -1848,11 +1872,13 @@ static void j1939_xtp_rx_dat_one(struct j1939_session *session, 
		j1939_tp_set_rxtimeout(session, 250);

	}

	session->last_cmd = 0xff;

	consume_skb(se_skb);

	j1939_session_put(session);



	return;



 out_session_cancel:

	kfree_skb(se_skb);

	j1939_session_timers_cancel(session);

	j1939_session_cancel(session, J1939_XTP_ABORT_FAULT);

	j1939_session_put(session);