query: bnx2 and tg3 don't check tcp and/or ip header length validity?

query: bnx2 and tg3 don't check tcp and/or ip header length validity?

[William Allen Simpson]

My question is whether it would be OK to add a simple test, and set it to
zero in case of bad values?

In both cases, they get a number that could be negative (in the case of a
badly formed header), and mash it into a flag vector of some sort.

No comments/documentation explaining purpose.

===

bnx2.c:
		u32 tcp_opt_len;
(ipv6 variant)
			vlan_tag_flags |= ((tcp_opt_len >> 2) << 8) |
					  TX_BD_FLAGS_SW_FLAGS;
(ipv4 variant)
			if (tcp_opt_len || (iph->ihl > 5)) {
				vlan_tag_flags |= ((iph->ihl - 5) +
						   (tcp_opt_len >> 2)) << 8;
			}

At least in the latter case, it bothers to check the IP header validity....

These are transmit-only, I cannot find where they use them on receive?

===

tg3.c:
		int tcp_opt_len, ip_tcp_len;

			tcp_opt_len = tcp_optlen(skb);
			ip_tcp_len = ip_hdrlen(skb) + sizeof(struct tcphdr);

			iph->check = 0;
			iph->tot_len = htons(mss + ip_tcp_len + tcp_opt_len);
			hdrlen = ip_tcp_len + tcp_opt_len;

...

		if (GET_ASIC_REV(tp->pci_chip_rev_id) == ASIC_REV_5717) {
			mss |= (hdrlen & 0xc) << 12;
			if (hdrlen & 0x10)
				base_flags |= 0x00000010;
			base_flags |= (hdrlen & 0x3e0) << 5;
		} else
			mss |= hdrlen << 9;

Likewise, transmit-only.  With completely different code later, in a dma
bug fix function.  But that's the overall picture....

Anybody have any idea what's going on here?

Re: query: bnx2 and tg3 don't check tcp and/or ip header length validity?

[William Allen Simpson]

William Allen Simpson wrote:
> My question is whether it would be OK to add a simple test, and set it to
> zero in case of bad values?
> 
Although both are compiled in my build, I've got no way to test them.  I'm
just going to do the easy thing and set to zero for now.  Somebody that
knows the code -- who should have done real error checking -- could actually
write better error checking and comments about the purpose of cramming the
length of the TCP option field into a tag....

-		tcp_opt_len = tcp_optlen(skb);
+		tcp_opt_len = tcp_option_len_th(tcp_hdr(skb));
+		if (tcp_opt_len < 0)
+			tcp_opt_len = 0;

Re: query: bnx2 and tg3 don't check tcp and/or ip header length validity?

[Michael Chan]

On Wed, 2009-10-14 at 13:46 -0700, William Allen Simpson wrote:
> William Allen Simpson wrote:
> > My question is whether it would be OK to add a simple test, and set it to
> > zero in case of bad values?
> > 
> Although both are compiled in my build, I've got no way to test them.  I'm
> just going to do the easy thing and set to zero for now.  Somebody that
> knows the code -- who should have done real error checking -- could actually
> write better error checking and comments about the purpose of cramming the
> length of the TCP option field into a tag....

The option length is needed by the hardware to segment a TSO packet into
proper MTU-sized packets.  You'll get malformed packets if the TSO
header is bad.  Setting it to zero perhaps can make these bad packets
more deterministic, but I don't know for sure.

> 
> -		tcp_opt_len = tcp_optlen(skb);
> +		tcp_opt_len = tcp_option_len_th(tcp_hdr(skb));
> +		if (tcp_opt_len < 0)
> +			tcp_opt_len = 0;
> 

Re: query: bnx2 and tg3 don't check tcp and/or ip header length validity?

[William Allen Simpson]

Michael Chan wrote:
> The option length is needed by the hardware to segment a TSO packet into
> proper MTU-sized packets.  You'll get malformed packets if the TSO
> header is bad.  Setting it to zero perhaps can make these bad packets
> more deterministic, but I don't know for sure.
> 
Malformed packets are unlikely (I'll use unlikely() on the test), but
I've seen a lot of unlikely things happen over the years.  When I was
concourse manager at Interop '91, a bad Portmaster build wouldn't pass
packets through one kind of router (3com); but it passed through all
the others!  Turned out, *most* routers didn't check the IP version
and IHL fields.  Shocking!

When we were designing IPv6 in '93, we had to use new IEEE numbers, etc.
(instead of the IP version and IHL) to distinguish the new version.
Otherwise, various printers crashed....

Unless there's a clearly documented check earlier in the code path (and
there's nothing documented here), always re-check everything.  (Also,
never forget cosmic radiation....)  Remember, from a driver developer's
perspective, the hardware always fails.  (And from a hardware viewpoint,
the software is always bad.)

Re: query: bnx2 and tg3 don't check tcp and/or ip header length validity?

[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.