Commit 38dc3b5f authored by Jakub Kicinski's avatar Jakub Kicinski
Browse files

Merge branch 'llc-fix-sk_buff-refcounting' 



Eric Biggers says:

====================
Patches 1-2 fix the memory leaks that syzbot has reported in net/llc:

	memory leak in llc_ui_create (2)
	memory leak in llc_ui_sendmsg
	memory leak in llc_conn_ac_send_sabme_cmd_p_set_x

Patches 3-4 fix related bugs that I noticed while reading this code.

Note: I've tested that this fixes the syzbot bugs, but otherwise I don't
know of any way to test this code.
====================

Signed-off-by: default avatarJakub Kicinski <jakub.kicinski@netronome.com>
parents 503c9add 36453c85

include/net/llc_conn.h

+1 −1
Original line number Diff line number Diff line
@@ -104,7 +104,7 @@ void llc_sk_reset(struct sock *sk); 


/* Access to a connection */

int llc_conn_state_process(struct sock *sk, struct sk_buff *skb);

int llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb);

void llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb);

void llc_conn_rtn_pdu(struct sock *sk, struct sk_buff *skb);

void llc_conn_resend_i_pdu_as_cmd(struct sock *sk, u8 nr, u8 first_p_bit);

void llc_conn_resend_i_pdu_as_rsp(struct sock *sk, u8 nr, u8 first_f_bit);

net/llc/af_llc.c

+20 −14
Original line number Diff line number Diff line
@@ -113,23 +113,27 @@ static inline u8 llc_ui_header_len(struct sock *sk, struct sockaddr_llc *addr) 
 *

 *	Send data via reliable llc2 connection.

 *	Returns 0 upon success, non-zero if action did not succeed.

 *

 *	This function always consumes a reference to the skb.

 */

static int llc_ui_send_data(struct sock* sk, struct sk_buff *skb, int noblock)

{

	struct llc_sock* llc = llc_sk(sk);

	int rc = 0;



	if (unlikely(llc_data_accept_state(llc->state) ||

		     llc->remote_busy_flag ||

		     llc->p_flag)) {

		long timeout = sock_sndtimeo(sk, noblock);

		int rc;



		rc = llc_ui_wait_for_busy_core(sk, timeout);

	}

	if (unlikely(!rc))

		rc = llc_build_and_send_pkt(sk, skb);

		if (rc) {

			kfree_skb(skb);

			return rc;

		}

	}

	return llc_build_and_send_pkt(sk, skb);

}



static void llc_ui_sk_init(struct socket *sock, struct sock *sk)

{
@@ -899,7 +903,7 @@ static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) 
	DECLARE_SOCKADDR(struct sockaddr_llc *, addr, msg->msg_name);

	int flags = msg->msg_flags;

	int noblock = flags & MSG_DONTWAIT;

	struct sk_buff *skb;

	struct sk_buff *skb = NULL;

	size_t size = 0;

	int rc = -EINVAL, copied = 0, hdrlen;
@@ -908,10 +912,10 @@ static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) 
	lock_sock(sk);

	if (addr) {

		if (msg->msg_namelen < sizeof(*addr))

			goto release;

			goto out;

	} else {

		if (llc_ui_addr_null(&llc->addr))

			goto release;

			goto out;

		addr = &llc->addr;

	}

	/* must bind connection to sap if user hasn't done it. */
@@ -919,7 +923,7 @@ static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) 
		/* bind to sap with null dev, exclusive. */

		rc = llc_ui_autobind(sock, addr);

		if (rc)

			goto release;

			goto out;

	}

	hdrlen = llc->dev->hard_header_len + llc_ui_header_len(sk, addr);

	size = hdrlen + len;
@@ -928,12 +932,12 @@ static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) 
	copied = size - hdrlen;

	rc = -EINVAL;

	if (copied < 0)

		goto release;

		goto out;

	release_sock(sk);

	skb = sock_alloc_send_skb(sk, size, noblock, &rc);

	lock_sock(sk);

	if (!skb)

		goto release;

		goto out;

	skb->dev      = llc->dev;

	skb->protocol = llc_proto_type(addr->sllc_arphrd);

	skb_reserve(skb, hdrlen);
@@ -943,29 +947,31 @@ static int llc_ui_sendmsg(struct socket *sock, struct msghdr *msg, size_t len) 
	if (sk->sk_type == SOCK_DGRAM || addr->sllc_ua) {

		llc_build_and_send_ui_pkt(llc->sap, skb, addr->sllc_mac,

					  addr->sllc_sap);

		skb = NULL;

		goto out;

	}

	if (addr->sllc_test) {

		llc_build_and_send_test_pkt(llc->sap, skb, addr->sllc_mac,

					    addr->sllc_sap);

		skb = NULL;

		goto out;

	}

	if (addr->sllc_xid) {

		llc_build_and_send_xid_pkt(llc->sap, skb, addr->sllc_mac,

					   addr->sllc_sap);

		skb = NULL;

		goto out;

	}

	rc = -ENOPROTOOPT;

	if (!(sk->sk_type == SOCK_STREAM && !addr->sllc_ua))

		goto out;

	rc = llc_ui_send_data(sk, skb, noblock);

	skb = NULL;

out:

	if (rc) {

	kfree_skb(skb);

release:

	if (rc)

		dprintk("%s: failed sending from %02X to %02X: %d\n",

			__func__, llc->laddr.lsap, llc->daddr.lsap, rc);

	}

	release_sock(sk);

	return rc ? : copied;

}

net/llc/llc_c_ac.c

+6 −2
Original line number Diff line number Diff line
@@ -372,6 +372,7 @@ int llc_conn_ac_send_i_cmd_p_set_1(struct sock *sk, struct sk_buff *skb) 
	llc_pdu_init_as_i_cmd(skb, 1, llc->vS, llc->vR);

	rc = llc_mac_hdr_init(skb, llc->dev->dev_addr, llc->daddr.mac);

	if (likely(!rc)) {

		skb_get(skb);

		llc_conn_send_pdu(sk, skb);

		llc_conn_ac_inc_vs_by_1(sk, skb);

	}
@@ -389,7 +390,8 @@ static int llc_conn_ac_send_i_cmd_p_set_0(struct sock *sk, struct sk_buff *skb) 
	llc_pdu_init_as_i_cmd(skb, 0, llc->vS, llc->vR);

	rc = llc_mac_hdr_init(skb, llc->dev->dev_addr, llc->daddr.mac);

	if (likely(!rc)) {

		rc = llc_conn_send_pdu(sk, skb);

		skb_get(skb);

		llc_conn_send_pdu(sk, skb);

		llc_conn_ac_inc_vs_by_1(sk, skb);

	}

	return rc;
@@ -406,6 +408,7 @@ int llc_conn_ac_send_i_xxx_x_set_0(struct sock *sk, struct sk_buff *skb) 
	llc_pdu_init_as_i_cmd(skb, 0, llc->vS, llc->vR);

	rc = llc_mac_hdr_init(skb, llc->dev->dev_addr, llc->daddr.mac);

	if (likely(!rc)) {

		skb_get(skb);

		llc_conn_send_pdu(sk, skb);

		llc_conn_ac_inc_vs_by_1(sk, skb);

	}
@@ -916,7 +919,8 @@ static int llc_conn_ac_send_i_rsp_f_set_ackpf(struct sock *sk, 
	llc_pdu_init_as_i_cmd(skb, llc->ack_pf, llc->vS, llc->vR);

	rc = llc_mac_hdr_init(skb, llc->dev->dev_addr, llc->daddr.mac);

	if (likely(!rc)) {

		rc = llc_conn_send_pdu(sk, skb);

		skb_get(skb);

		llc_conn_send_pdu(sk, skb);

		llc_conn_ac_inc_vs_by_1(sk, skb);

	}

	return rc;

net/llc/llc_conn.c

+17 −50
Original line number Diff line number Diff line
@@ -30,7 +30,7 @@ 
#endif



static int llc_find_offset(int state, int ev_type);

static int llc_conn_send_pdus(struct sock *sk, struct sk_buff *skb);

static void llc_conn_send_pdus(struct sock *sk);

static int llc_conn_service(struct sock *sk, struct sk_buff *skb);

static int llc_exec_conn_trans_actions(struct sock *sk,

				       struct llc_conn_state_trans *trans,
@@ -55,6 +55,8 @@ int sysctl_llc2_busy_timeout = LLC2_BUSY_TIME * HZ; 
 *	(executing it's actions and changing state), upper layer will be

 *	indicated or confirmed, if needed. Returns 0 for success, 1 for

 *	failure. The socket lock has to be held before calling this function.

 *

 *	This function always consumes a reference to the skb.

 */

int llc_conn_state_process(struct sock *sk, struct sk_buff *skb)

{
@@ -62,12 +64,6 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb) 
	struct llc_sock *llc = llc_sk(skb->sk);

	struct llc_conn_state_ev *ev = llc_conn_ev(skb);



	/*

	 * We have to hold the skb, because llc_conn_service will kfree it in

	 * the sending path and we need to look at the skb->cb, where we encode

	 * llc_conn_state_ev.

	 */

	skb_get(skb);

	ev->ind_prim = ev->cfm_prim = 0;

	/*

	 * Send event to state machine
@@ -75,21 +71,12 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb) 
	rc = llc_conn_service(skb->sk, skb);

	if (unlikely(rc != 0)) {

		printk(KERN_ERR "%s: llc_conn_service failed\n", __func__);

		goto out_kfree_skb;

	}



	if (unlikely(!ev->ind_prim && !ev->cfm_prim)) {

		/* indicate or confirm not required */

		if (!skb->next)

			goto out_kfree_skb;

		goto out_skb_put;

	}



	if (unlikely(ev->ind_prim && ev->cfm_prim)) /* Paranoia */

		skb_get(skb);



	switch (ev->ind_prim) {

	case LLC_DATA_PRIM:

		skb_get(skb);

		llc_save_primitive(sk, skb, LLC_DATA_PRIM);

		if (unlikely(sock_queue_rcv_skb(sk, skb))) {

			/*
@@ -106,6 +93,7 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb) 
		 * skb->sk pointing to the newly created struct sock in

		 * llc_conn_handler. -acme

		 */

		skb_get(skb);

		skb_queue_tail(&sk->sk_receive_queue, skb);

		sk->sk_state_change(sk);

		break;
@@ -121,7 +109,6 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb) 
				sk->sk_state_change(sk);

			}

		}

		kfree_skb(skb);

		sock_put(sk);

		break;

	case LLC_RESET_PRIM:
@@ -130,14 +117,11 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb) 
		 * RESET is not being notified to upper layers for now

		 */

		printk(KERN_INFO "%s: received a reset ind!\n", __func__);

		kfree_skb(skb);

		break;

	default:

		if (ev->ind_prim) {

		if (ev->ind_prim)

			printk(KERN_INFO "%s: received unknown %d prim!\n",

				__func__, ev->ind_prim);

			kfree_skb(skb);

		}

		/* No indication */

		break;

	}
@@ -179,25 +163,22 @@ int llc_conn_state_process(struct sock *sk, struct sk_buff *skb) 
		printk(KERN_INFO "%s: received a reset conf!\n", __func__);

		break;

	default:

		if (ev->cfm_prim) {

		if (ev->cfm_prim)

			printk(KERN_INFO "%s: received unknown %d prim!\n",

					__func__, ev->cfm_prim);

		/* No confirmation */

		break;

	}

		goto out_skb_put; /* No confirmation */

	}

out_kfree_skb:

	kfree_skb(skb);

out_skb_put:

	kfree_skb(skb);

	return rc;

}



int llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb)

void llc_conn_send_pdu(struct sock *sk, struct sk_buff *skb)

{

	/* queue PDU to send to MAC layer */

	skb_queue_tail(&sk->sk_write_queue, skb);

	return llc_conn_send_pdus(sk, skb);

	llc_conn_send_pdus(sk);

}



/**
@@ -255,7 +236,7 @@ void llc_conn_resend_i_pdu_as_cmd(struct sock *sk, u8 nr, u8 first_p_bit) 
	if (howmany_resend > 0)

		llc->vS = (llc->vS + 1) % LLC_2_SEQ_NBR_MODULO;

	/* any PDUs to re-send are queued up; start sending to MAC */

	llc_conn_send_pdus(sk, NULL);

	llc_conn_send_pdus(sk);

out:;

}
@@ -296,7 +277,7 @@ void llc_conn_resend_i_pdu_as_rsp(struct sock *sk, u8 nr, u8 first_f_bit) 
	if (howmany_resend > 0)

		llc->vS = (llc->vS + 1) % LLC_2_SEQ_NBR_MODULO;

	/* any PDUs to re-send are queued up; start sending to MAC */

	llc_conn_send_pdus(sk, NULL);

	llc_conn_send_pdus(sk);

out:;

}
@@ -340,16 +321,12 @@ out: 
/**

 *	llc_conn_send_pdus - Sends queued PDUs

 *	@sk: active connection

 *	@hold_skb: the skb held by caller, or NULL if does not care

 *

 *	Sends queued pdus to MAC layer for transmission. When @hold_skb is

 *	NULL, always return 0. Otherwise, return 0 if @hold_skb is sent

 *	successfully, or 1 for failure.

 *	Sends queued pdus to MAC layer for transmission.

 */

static int llc_conn_send_pdus(struct sock *sk, struct sk_buff *hold_skb)

static void llc_conn_send_pdus(struct sock *sk)

{

	struct sk_buff *skb;

	int ret = 0;



	while ((skb = skb_dequeue(&sk->sk_write_queue)) != NULL) {

		struct llc_pdu_sn *pdu = llc_pdu_sn_hdr(skb);
@@ -361,20 +338,10 @@ static int llc_conn_send_pdus(struct sock *sk, struct sk_buff *hold_skb) 
			skb_queue_tail(&llc_sk(sk)->pdu_unack_q, skb);

			if (!skb2)

				break;

			dev_queue_xmit(skb2);

		} else {

			bool is_target = skb == hold_skb;

			int rc;



			if (is_target)

				skb_get(skb);

			rc = dev_queue_xmit(skb);

			if (is_target)

				ret = rc;

			skb = skb2;

		}

		dev_queue_xmit(skb);

	}



	return ret;

}



/**

net/llc/llc_if.c

+8 −4
Original line number Diff line number Diff line
@@ -38,6 +38,8 @@ 
 *	closed and -EBUSY when sending data is not permitted in this state or

 *	LLC has send an I pdu with p bit set to 1 and is waiting for it's

 *	response.

 *

 *	This function always consumes a reference to the skb.

 */

int llc_build_and_send_pkt(struct sock *sk, struct sk_buff *skb)

{
@@ -46,20 +48,22 @@ int llc_build_and_send_pkt(struct sock *sk, struct sk_buff *skb) 
	struct llc_sock *llc = llc_sk(sk);



	if (unlikely(llc->state == LLC_CONN_STATE_ADM))

		goto out;

		goto out_free;

	rc = -EBUSY;

	if (unlikely(llc_data_accept_state(llc->state) || /* data_conn_refuse */

		     llc->p_flag)) {

		llc->failed_data_req = 1;

		goto out;

		goto out_free;

	}

	ev = llc_conn_ev(skb);

	ev->type      = LLC_CONN_EV_TYPE_PRIM;

	ev->prim      = LLC_DATA_PRIM;

	ev->prim_type = LLC_PRIM_TYPE_REQ;

	skb->dev      = llc->dev;

	rc = llc_conn_state_process(sk, skb);

out:

	return llc_conn_state_process(sk, skb);



out_free:

	kfree_skb(skb);

	return rc;

}

CRA Git | Maintained and supported by SUSTech CRA and CCSE