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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20120223.173305.576493449692097140.davem@davemloft.net>
Date:	Thu, 23 Feb 2012 17:33:05 -0500 (EST)
From:	David Miller <davem@...emloft.net>
To:	ajuncu@...acom.com
Cc:	acme@...stprotocols.net, netdev@...r.kernel.org,
	dbaluta@...acom.com, alexj@...edu.org
Subject: Re: [PATCH] llc: fix skb_over_panic due to bogus RX packet

From: Alexandru Juncu <ajuncu@...acom.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html