Message ID | 1329819196-25176-1-git-send-email-ajuncu@ixiacom.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)