6451861 potential buffer overflow when GLDv3 ACKs DL_BIND_REQ messages

6483982 potential memory corruption in GLDv3 packet header parsing logic
6486658 GLDv3's careless use of b_next can trigger freeb()/freemsg() ASSERT()

2 files changed, 19 insertions, 11 deletions

diff --git a/usr/src/uts/common/io/dld/dld_proto.c b/usr/src/uts/common/io/dld/dld_proto.c
index 3d5db8d82e..994ad7af38 100644
--- a/usr/src/uts/common/io/dld/dld_proto.c
+++ b/usr/src/uts/common/io/dld/dld_proto.c
@@ -484,8 +484,8 @@ proto_bind_req(dld_str_t *dsp, union DL_primitives *udlp, mblk_t *mp)
 {
 	dl_bind_req_t	*dlp = (dl_bind_req_t *)udlp;
 	int		err = 0;
-	uint8_t		addr[MAXMACADDRLEN];
-	uint_t		addr_length;
+	uint8_t		dlsap_addr[MAXMACADDRLEN + sizeof (uint16_t)];
+	uint_t		dlsap_addr_length;
 	t_uscalar_t	dl_err;
 	t_scalar_t	sap;
 	queue_t		*q = dsp->ds_wq;
@@ -551,14 +551,14 @@ proto_bind_req(dld_str_t *dsp, union DL_primitives *udlp, mblk_t *mp)
 	/*
 	 * Copy in MAC address.
 	 */
-	addr_length = dsp->ds_mip->mi_addr_length;
-	bcopy(dsp->ds_curr_addr, addr, addr_length);
+	dlsap_addr_length = dsp->ds_mip->mi_addr_length;
+	bcopy(dsp->ds_curr_addr, dlsap_addr, dlsap_addr_length);
 
 	/*
-	 * Copy in the DLSAP.
+	 * Copy in the SAP.
 	 */
-	*(uint16_t *)(addr + addr_length) = dsp->ds_sap;
-	addr_length += sizeof (uint16_t);
+	*(uint16_t *)(dlsap_addr + dlsap_addr_length) = dsp->ds_sap;
+	dlsap_addr_length += sizeof (uint16_t);
 
 	dsp->ds_dlstate = DL_IDLE;
 	if (dsp->ds_passivestate == DLD_UNINITIALIZED)
@@ -566,7 +566,7 @@ proto_bind_req(dld_str_t *dsp, union DL_primitives *udlp, mblk_t *mp)
 
 	rw_exit(&dsp->ds_lock);
 
-	dlbindack(q, mp, sap, (void *)addr, addr_length, 0, 0);
+	dlbindack(q, mp, sap, dlsap_addr, dlsap_addr_length, 0, 0);
 	return (B_TRUE);
 failed:
 	rw_exit(&dsp->ds_lock);
diff --git a/usr/src/uts/common/io/dls/dls_link.c b/usr/src/uts/common/io/dls/dls_link.c
index 19e2f12976..8ecdcdd7f5 100644
--- a/usr/src/uts/common/io/dls/dls_link.c
+++ b/usr/src/uts/common/io/dls/dls_link.c
@@ -122,9 +122,12 @@ i_dls_link_destructor(void *buf, void *arg)
  * - Parse the mac header information of the given packet.
  * - Strip the padding and skip over the header. Note that because some
  *   DLS consumers only check the db_ref count of the first mblk, we
- *   pullup the message into a single mblk. The dls_link_header_info()
- *   function ensures that the size of the pulled message is greater
- *   than the MAC header size.
+ *   pullup the message into a single mblk. Because the original message
+ *   is freed as the result of message pulling up, dls_link_header_info()
+ *   is called again to update the mhi_saddr and mhi_daddr pointers in the
+ *   mhip. Further, the dls_link_header_info() function ensures that the
+ *   size of the pulled message is greater than the MAC header size,
+ *   therefore we can directly advance b_rptr to point at the payload.
  *
  * We choose to use a macro for performance reasons.
  */
@@ -137,8 +140,11 @@ i_dls_link_destructor(void *buf, void *arg)
 			if ((newmp = msgpullup((mp), -1)) == NULL) {	\
 				(err) = EINVAL;				\
 			} else {					\
+				(mp)->b_next = NULL;			\
 				freemsg((mp));				\
 				(mp) = newmp;				\
+				VERIFY(dls_link_header_info((dlp),	\
+				    (mp), (mhip)) == 0);		\
 				(mp)->b_next = nextp;			\
 				(mp)->b_rptr += (mhip)->mhi_hdrsize;	\
 			}						\
@@ -360,6 +366,7 @@ i_dls_link_rx(void *arg, mac_resource_handle_t mrh, mblk_t *mp)
 		if (err != 0) {
 			atomic_add_32(&(dlp->dl_unknowns), 1);
 			nextp = mp->b_next;
+			mp->b_next = NULL;
 			freemsg(mp);
 			continue;
 		}
@@ -554,6 +561,7 @@ i_dls_link_rx_common(void *arg, mac_resource_handle_t mrh, mblk_t *mp,
 			if (acceptfunc == dls_accept)
 				atomic_add_32(&(dlp->dl_unknowns), 1);
 			nextp = mp->b_next;
+			mp->b_next = NULL;
 			freemsg(mp);
 			continue;
 		}