| 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
| ||
|
Message-ID: <20100312174656.GA12175@bicker>
Date: Fri, 12 Mar 2010 20:46:56 +0300
From: Dan Carpenter <error27@...il.com>
To: William Allen Simpson <william.allen.simpson@...il.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Linux Kernel Developers <linux-kernel@...r.kernel.org>,
Linux Kernel Network Developers <netdev@...r.kernel.org>,
David Miller <davem@...emloft.net>,
Michael Chan <mchan@...adcom.com>,
Simon Horman <horms@...ge.net.au>
Subject: Re: [PATCH v4 2/7] net: remove old tcp_optlen function
On Fri, Mar 12, 2010 at 08:25:16AM -0500, William Allen Simpson wrote:
> The tcp_optlen() function returns a potential *negative* unsigned.
>
> In the only two existing files using the old tcp_optlen() function,
> clean up confusing and inconsistent mixing of both byte and word
> offsets, and other coding style issues. Document assumptions.
>
> Quoth David Miller:
> This is transmit, and the packets can only come from the Linux
> TCP stack, not some external entity.
>
> You're being way too anal here, and adding these checks to
> drivers would be just a lot of rediculious bloat. [sic]
>
> Therefore, there are *no* checks for bad TCP and IP header sizes, nor
> any semantic changes. The drivers should function exactly as existing,
> although usage of int should ameliorate the issues.
>
So after you removed the checks this change includes:
1) Random slagging on the networking guys
2) u32 => int to ameliorate your static checker's complaints
3) cleanups
People have already explained that tcp_optlen() doesn't return
negative values. It would really help us if you could show how
tcp_hdr(skb)->doff can be less than 5?
regards,
dan carpenter
> Requires:
> net: tcp_header_len_th and tcp_option_len_th
>
> Signed-off-by: William.Allen.Simpson@...il.com
> CC: Michael Chan <mchan@...adcom.com>
> ---
> drivers/net/bnx2.c | 29 +++++++++++++-----------
> drivers/net/tg3.c | 60 +++++++++++++++++++++++---------------------------
> include/linux/tcp.h | 5 ----
> 3 files changed, 44 insertions(+), 50 deletions(-)
>
> No response from testers in 21+ weeks.
>
> [removed comment references to commit log]
>
> diff --git a/drivers/net/bnx2.c b/drivers/net/bnx2.c
> index 381887b..87607b8 100644
> --- a/drivers/net/bnx2.c
> +++ b/drivers/net/bnx2.c
> @@ -6335,6 +6335,8 @@ bnx2_vlan_rx_register(struct net_device *dev, struct vlan_group *vlgrp)
> /* Called with netif_tx_lock.
> * bnx2_tx_int() runs without netif_tx_lock unless it needs to call
> * netif_wake_queue().
> + *
> + * No TCP or IP length checking, as required by David Miller.
> */
> static netdev_tx_t
> bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
> @@ -6378,19 +6380,19 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
> (TX_BD_FLAGS_VLAN_TAG | (vlan_tx_tag_get(skb) << 16));
> }
> #endif
> - if ((mss = skb_shinfo(skb)->gso_size)) {
> - u32 tcp_opt_len;
> - struct iphdr *iph;
> + mss = skb_shinfo(skb)->gso_size;
> + if (mss != 0) {
> + struct tcphdr *th = tcp_hdr(skb);
> + int tcp_opt_words = th->doff - (sizeof(*th) >> 2);
> + /* assumes positive tcp_opt_words without checking */
>
> vlan_tag_flags |= TX_BD_FLAGS_SW_LSO;
>
> - tcp_opt_len = tcp_optlen(skb);
> -
> if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6) {
> u32 tcp_off = skb_transport_offset(skb) -
> sizeof(struct ipv6hdr) - ETH_HLEN;
>
> - vlan_tag_flags |= ((tcp_opt_len >> 2) << 8) |
> + vlan_tag_flags |= (tcp_opt_words << 8) |
> TX_BD_FLAGS_SW_FLAGS;
> if (likely(tcp_off == 0))
> vlan_tag_flags &= ~TX_BD_FLAGS_TCP6_OFF0_MSK;
> @@ -6403,14 +6405,15 @@ bnx2_start_xmit(struct sk_buff *skb, struct net_device *dev)
> mss |= (tcp_off & 0xc) << TX_BD_TCP6_OFF2_SHL;
> }
> } else {
> - iph = ip_hdr(skb);
> - if (tcp_opt_len || (iph->ihl > 5)) {
> - vlan_tag_flags |= ((iph->ihl - 5) +
> - (tcp_opt_len >> 2)) << 8;
> - }
> + struct iphdr *iph = ip_hdr(skb);
> + int ip_opt_words = iph->ihl - (sizeof(*iph) >> 2);
> + /* assumes positive ip_opt_words without checking */
> + int opt_words = ip_opt_words + tcp_opt_words;
> +
> + if (opt_words > 0)
> + vlan_tag_flags |= opt_words << 8;
> }
> - } else
> - mss = 0;
> + }
>
> mapping = pci_map_single(bp->pdev, skb->data, len, PCI_DMA_TODEVICE);
> if (pci_dma_mapping_error(bp->pdev, mapping)) {
> diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
> index 0fa7688..6ad8184 100644
> --- a/drivers/net/tg3.c
> +++ b/drivers/net/tg3.c
> @@ -5481,6 +5481,8 @@ static void tg3_set_txd(struct tg3_napi *tnapi, int entry,
>
> /* hard_start_xmit for devices that don't have any bugs and
> * support TG3_FLG2_HW_TSO_2 and TG3_FLG2_HW_TSO_3 only.
> + *
> + * No TCP or IP length checking, as required by David Miller.
> */
> static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
> struct net_device *dev)
> @@ -5515,9 +5517,9 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
>
> entry = tnapi->tx_prod;
> base_flags = 0;
> - mss = 0;
> - if ((mss = skb_shinfo(skb)->gso_size) != 0) {
> - int tcp_opt_len, ip_tcp_len;
> + mss = skb_shinfo(skb)->gso_size;
> + if (mss != 0) {
> + struct tcphdr *th;
> u32 hdrlen;
>
> if (skb_header_cloned(skb) &&
> @@ -5525,18 +5527,16 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
> dev_kfree_skb(skb);
> goto out_unlock;
> }
> + th = tcp_hdr(skb);
>
> if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
> hdrlen = skb_headlen(skb) - ETH_HLEN;
> else {
> struct iphdr *iph = ip_hdr(skb);
>
> - tcp_opt_len = tcp_optlen(skb);
> - ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
> -
> + hdrlen = ip_hdrlen(skb) + tcp_header_len_th(th);
> + iph->tot_len = htons(mss + hdrlen);
> iph->check = 0;
> - iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len);
> - hdrlen = ip_tcp_len + tcp_opt_len;
> }
>
> if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) {
> @@ -5550,7 +5550,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
> base_flags |= (TXD_FLAG_CPU_PRE_DMA |
> TXD_FLAG_CPU_POST_DMA);
>
> - tcp_hdr(skb)->check = 0;
> + th->check = 0;
>
> }
> else if (skb->ip_summed == CHECKSUM_PARTIAL)
> @@ -5683,6 +5683,8 @@ tg3_tso_bug_end:
>
> /* hard_start_xmit for devices that have the 4G bug and/or 40-bit bug and
> * support TG3_FLG2_HW_TSO_1 or firmware TSO only.
> + *
> + * No TCP or IP length checking, as required by David Miller.
> */
> static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
> struct net_device *dev)
> @@ -5721,20 +5723,21 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
> if (skb->ip_summed == CHECKSUM_PARTIAL)
> base_flags |= TXD_FLAG_TCPUDP_CSUM;
>
> - if ((mss = skb_shinfo(skb)->gso_size) != 0) {
> + mss = skb_shinfo(skb)->gso_size;
> + if (mss != 0) {
> struct iphdr *iph;
> - u32 tcp_opt_len, ip_tcp_len, hdr_len;
> + struct tcphdr *th;
> + u32 hdr_len;
> + int opt_bytes;
>
> if (skb_header_cloned(skb) &&
> pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
> dev_kfree_skb(skb);
> goto out_unlock;
> }
> + th = tcp_hdr(skb);
> + hdr_len = ip_hdrlen(skb) + tcp_header_len_th(th);
>
> - tcp_opt_len = tcp_optlen(skb);
> - ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);
> -
> - hdr_len = ip_tcp_len + tcp_opt_len;
> if (unlikely((ETH_HLEN + hdr_len) > 80) &&
> (tp->tg3_flags2 & TG3_FLG2_TSO_BUG))
> return (tg3_tso_bug(tp, skb));
> @@ -5746,13 +5749,14 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
> iph->check = 0;
> iph->tot_len = htons(mss + hdr_len);
> if (tp->tg3_flags2 & TG3_FLG2_HW_TSO) {
> - tcp_hdr(skb)->check = 0;
> + th->check = 0;
> base_flags &= ~TXD_FLAG_TCPUDP_CSUM;
> } else
> - tcp_hdr(skb)->check = ~csum_tcpudp_magic(iph->saddr,
> - iph->daddr, 0,
> - IPPROTO_TCP,
> - 0);
> + th->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
> + 0, IPPROTO_TCP, 0);
> +
> + opt_bytes = hdr_len - sizeof(*iph) - sizeof(*th);
> + /* assumes positive opt_bytes without checking */
>
> if (tp->tg3_flags2 & TG3_FLG2_HW_TSO_3) {
> mss |= (hdr_len & 0xc) << 12;
> @@ -5763,19 +5767,11 @@ static netdev_tx_t tg3_start_xmit_dma_bug(struct sk_buff *skb,
> mss |= hdr_len << 9;
> else if ((tp->tg3_flags2 & TG3_FLG2_HW_TSO_1) ||
> GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5705) {
> - if (tcp_opt_len || iph->ihl > 5) {
> - int tsflags;
> -
> - tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
> - mss |= (tsflags << 11);
> - }
> + if (opt_bytes > 0)
> + mss |= opt_bytes << (11 - 2);
> } else {
> - if (tcp_opt_len || iph->ihl > 5) {
> - int tsflags;
> -
> - tsflags = (iph->ihl - 5) + (tcp_opt_len >> 2);
> - base_flags |= tsflags << 12;
> - }
> + if (opt_bytes > 0)
> + base_flags |= opt_bytes << (12 - 2);
> }
> }
> #if TG3_VLAN_TAG_USED
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index 48ddeb8..854ad65 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -220,11 +220,6 @@ static inline unsigned int tcp_hdrlen(const struct sk_buff *skb)
> return tcp_hdr(skb)->doff * 4;
> }
>
> -static inline unsigned int tcp_optlen(const struct sk_buff *skb)
> -{
> - return (tcp_hdr(skb)->doff - 5) * 4;
> -}
> -
> /* Length of fixed header plus standard options. */
> static inline unsigned int tcp_header_len_th(const struct tcphdr *th)
> {
> --
> 1.6.3.3
>
--
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
Powered by blists - more mailing lists