[PATCH] llc: fix skb_over_panic due to bogus RX packet

[PATCH] llc: fix skb_over_panic due to bogus RX packet

[Alexandru Juncu]

Verify size of received packet so that no illegal access is made.

Signed-off-by: Alexandru Juncu <ajuncu@ixiacom.com>
---
 include/net/llc_pdu.h |   10 ++++++++--
 net/llc/llc_s_ac.c    |   12 +++++++++---
 net/llc/llc_station.c |    4 +++-
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/net/llc_pdu.h b/include/net/llc_pdu.h
index f57e7d4..9c3ed6c 100644
--- a/include/net/llc_pdu.h
+++ b/include/net/llc_pdu.h
@@ -14,6 +14,7 @@
 
 #include <linux/if_ether.h>
 #include <linux/if_tr.h>
+#include <net/sock.h>
 
 /* Lengths of frame formats */
 #define LLC_PDU_LEN_I	4       /* header and 2 control bytes */
@@ -336,7 +337,7 @@ static inline void llc_pdu_init_as_test_cmd(struct sk_buff *skb)
  *
  *	Builds a pdu frame as a TEST response.
  */
-static inline void llc_pdu_init_as_test_rsp(struct sk_buff *skb,
+static inline int llc_pdu_init_as_test_rsp(struct sk_buff *skb,
 					    struct sk_buff *ev_skb)
 {
 	struct llc_pdu_un *pdu = llc_pdu_un_hdr(skb);
@@ -348,10 +349,15 @@ static inline void llc_pdu_init_as_test_rsp(struct sk_buff *skb,
 		struct llc_pdu_un *ev_pdu = llc_pdu_un_hdr(ev_skb);
 		int dsize;
 
-		dsize = ntohs(eth_hdr(ev_skb)->h_proto) - 3;
+		dsize = ntohs(eth_hdr(ev_skb)->h_proto);
+		if((dsize < 3) || (dsize > skb_tailroom(skb))) {
+			return -EINVAL;
+		}
+		dsize -= 3;
 		memcpy(((u8 *)pdu) + 3, ((u8 *)ev_pdu) + 3, dsize);
 		skb_put(skb, dsize);
 	}
+	return 0;
 }
 
 /* LLC Type 1 XID command/response information fields format */
diff --git a/net/llc/llc_s_ac.c b/net/llc/llc_s_ac.c
index a94bd56..0972590 100644
--- a/net/llc/llc_s_ac.c
+++ b/net/llc/llc_s_ac.c
@@ -158,12 +158,18 @@ int llc_sap_action_send_test_r(struct llc_sap *sap, struct sk_buff *skb)
 		goto out;
 	llc_pdu_header_init(nskb, LLC_PDU_TYPE_U, sap->laddr.lsap, dsap,
 			    LLC_PDU_RSP);
-	llc_pdu_init_as_test_rsp(nskb, skb);
+	rc = llc_pdu_init_as_test_rsp(nskb, skb);
+	if (unlikely(rc))
+		goto free;
 	rc = llc_mac_hdr_init(nskb, mac_sa, mac_da);
-	if (likely(!rc))
-		rc = dev_queue_xmit(nskb);
+	if (unlikely(rc))
+		goto free;
+	rc = dev_queue_xmit(nskb);
 out:
 	return rc;
+free:
+	kfree_skb(nskb);
+	goto out;
 }
 
 /**
diff --git a/net/llc/llc_station.c b/net/llc/llc_station.c
index cf4aea3..f3a92d1 100644
--- a/net/llc/llc_station.c
+++ b/net/llc/llc_station.c
@@ -314,7 +314,9 @@ static int llc_station_ac_send_test_r(struct sk_buff *skb)
 	llc_pdu_decode_sa(skb, mac_da);
 	llc_pdu_decode_ssap(skb, &dsap);
 	llc_pdu_header_init(nskb, LLC_PDU_TYPE_U, 0, dsap, LLC_PDU_RSP);
-	llc_pdu_init_as_test_rsp(nskb, skb);
+	rc = llc_pdu_init_as_test_rsp(nskb, skb);
+	if (unlikely(rc))
+		goto free;
 	rc = llc_mac_hdr_init(nskb, skb->dev->dev_addr, mac_da);
 	if (unlikely(rc))
 		goto free;
-- 
1.7.5.4

Re: [PATCH] llc: fix skb_over_panic due to bogus RX packet

[David Miller]

From: Alexandru Juncu <ajuncu@ixiacom.com>
Date: Tue, 21 Feb 2012 12:13:16 +0200

> @@ -336,7 +337,7 @@ static inline void llc_pdu_init_as_test_cmd(struct sk_buff *skb)
>   *
>   *	Builds a pdu frame as a TEST response.
>   */
> -static inline void llc_pdu_init_as_test_rsp(struct sk_buff *skb,
> +static inline int llc_pdu_init_as_test_rsp(struct sk_buff *skb,
>  					    struct sk_buff *ev_skb)
>  {
>  	struct llc_pdu_un *pdu = llc_pdu_un_hdr(skb);
> @@ -348,10 +349,15 @@ static inline void llc_pdu_init_as_test_rsp(struct sk_buff *skb,
>  		struct llc_pdu_un *ev_pdu = llc_pdu_un_hdr(ev_skb);
>  		int dsize;
>  
> -		dsize = ntohs(eth_hdr(ev_skb)->h_proto) - 3;
> +		dsize = ntohs(eth_hdr(ev_skb)->h_proto);
> +		if((dsize < 3) || (dsize > skb_tailroom(skb))) {
> +			return -EINVAL;
> +		}
> +		dsize -= 3;
>  		memcpy(((u8 *)pdu) + 3, ((u8 *)ev_pdu) + 3, dsize);
>  		skb_put(skb, dsize);
>  	}
> +	return 0;
>  }

All callers of llc_pdu_init_as_test_rsp() allocate "skb" such that it
has size enough to accomodate what is present in ->h_proto.

The two call sites are:

	data_size = ntohs(eth_hdr(skb)->h_proto) - 3;
	nskb = llc_alloc_frame(NULL, skb->dev, LLC_PDU_TYPE_U, data_size);
	if (!nskb)
		goto out;
	llc_pdu_header_init(nskb, LLC_PDU_TYPE_U, sap->laddr.lsap, dsap,
			    LLC_PDU_RSP);
	llc_pdu_init_as_test_rsp(nskb, skb);

and

	data_size = ntohs(eth_hdr(skb)->h_proto) - 3;
	nskb = llc_alloc_frame(NULL, skb->dev, LLC_PDU_TYPE_U, data_size);

	if (!nskb)
		goto out;
	rc = 0;
	llc_pdu_decode_sa(skb, mac_da);
	llc_pdu_decode_ssap(skb, &dsap);
	llc_pdu_header_init(nskb, LLC_PDU_TYPE_U, 0, dsap, LLC_PDU_RSP);
	llc_pdu_init_as_test_rsp(nskb, skb);

Therefore I cannot see a case where the SKB's space is insufficient for
the pdu header copy performed by llc_pdu_init_as_test_rsp().

And if anything this indicates that the call sites should validate the
dsize first before doing all of the expensive memory allocations and
PDU header initialization.  These little header munging routines like
llc_pdu_init_as_test_rsp() shouldn't have to validate their arguments,
they should be simple and have a sane validated state given to them.

Also, even if this change was actually needed, your coding style needs
to be fixed:

+		if((dsize < 3) || (dsize > skb_tailroom(skb))) {
+			return -EINVAL;
+		}

Should be:

		if (dsize < 3 || dsize > skb_tailroom(skb))
			return -EINVAL;

And your commit message was too terse, you need to describe exactly
how the over panic can happen, otherwise I have to analyze it myself
and if I can't figure it out then I have to ask you all of the
questions which should be answered from the start in your change
description.