From owner-freebsd-net Thu Dec 21 15:37: 9 2000 From owner-freebsd-net@FreeBSD.ORG Thu Dec 21 15:37:03 2000 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from gatekeeper.tsc.tdk.com (gatekeeper.tsc.tdk.com [207.113.159.21]) by hub.freebsd.org (Postfix) with ESMTP id 8424637B400; Thu, 21 Dec 2000 15:37:02 -0800 (PST) Received: from imap.gv.tsc.tdk.com (imap.gv.tsc.tdk.com [192.168.241.198]) by gatekeeper.tsc.tdk.com (8.8.8/8.8.8) with ESMTP id PAA29267; Thu, 21 Dec 2000 15:36:54 -0800 (PST) (envelope-from gdonl@tsc.tdk.com) Received: from salsa.gv.tsc.tdk.com (salsa.gv.tsc.tdk.com [192.168.241.194]) by imap.gv.tsc.tdk.com (8.9.3/8.9.3) with ESMTP id PAA10256; Thu, 21 Dec 2000 15:36:53 -0800 (PST) (envelope-from Don.Lewis@tsc.tdk.com) Received: (from gdonl@localhost) by salsa.gv.tsc.tdk.com (8.8.5/8.8.5) id PAA27025; Thu, 21 Dec 2000 15:36:52 -0800 (PST) From: Don Lewis Message-Id: <200012212336.PAA27025@salsa.gv.tsc.tdk.com> Date: Thu, 21 Dec 2000 15:36:52 -0800 In-Reply-To: <20001220155118.N81814@skriver.dk> References: <20001218182600.C1856@skriver.dk> <20001219222730.A29741@skriver.dk> <200012201046.CAA19456@salsa.gv.tsc.tdk.com> <20001220155118.N81814@skriver.dk> X-Mailer: Mail User's Shell (7.2.6 beta(5) 10/07/98) To: Jesper Skriver , Don Lewis Subject: Re: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c tcp_subr.c tcp_var.h Cc: Kris Kennaway , Poul-Henning Kamp , security-officer@FreeBSD.ORG, cvs-all@FreeBSD.ORG, freebsd-net@FreeBSD.ORG Sender: gdonl@tsc.tdk.com Sender: owner-freebsd-net@FreeBSD.ORG Precedence: bulk X-Loop: FreeBSD.org On Dec 20, 3:51pm, Jesper Skriver wrote: } Subject: Re: what to do now ? Was: cvs commit: src/sys/netinet ip_icmp.c } On Wed, Dec 20, 2000 at 02:46:21AM -0800, Don Lewis wrote: } } > } It has the following functionality. } > } } > } - If the sysctl net.inet.tcp.icmp_admin_prohib_like_rst == 1 (default) } > } it enables the below. } > } - When a ICMP administrative prohibited is recieved, it check is the } > } IP header attached to the ICMP packet has any options set, if it has } > } it ignores it. The reason for this is, if any options is set the extra } > } 8 bytes is no longer the first 8 bytes from the TCP header, source/ } > } destination ports and sequence number, which we need to find the } > } right TCP session. } > } > According to Stevens, we should get the first 8 bytes of the TCP header } > even if there are options on the ICMP packet. We would have to be } > careful to do sanity checking in this case, as well as guard against } > unaligned accesses to the TCP header data. } } I'll read more on this, for now I think it's a improvement to ignore all } packets with IP options, then we can improve it later by handling } packets with options too. I would expect the option-less case to be the most common, so it's ok to defer this. } > } @@ -714,6 +715,15 @@ } > } (lport && inp->inp_lport != lport) || } > } (laddr.s_addr && inp->inp_laddr.s_addr != laddr.s_addr) || } > } (fport && inp->inp_fport != fport)) { } > } + inp = inp->inp_list.le_next; } > } + continue; } > } > Wouldn't it be more cleaner (gets rid of the loop) and more efficient (if } > we're getting blasted with ICMP messages) to use in_pcblookup_hash()? } } I didn't change the loop, but I'll have a look at this code, to see if } we can improve it, but again, to get moving, I'd like to commit this, } and leave this for a later improvement, ok ? Sure. } > } + } } > } + /* } > } + * If tcp_sequence is set, then skip sessions where } > } + * the sequence number is not one of a unacknowledged } > } + * packet. } > } + */ } > } + if ((tcp_sequence) && (tcp_seq_vs_sess(inp, tcp_sequence) == 0)) { } > } inp = inp->inp_list.le_next; } > } continue; } > } > We should pass in an extra flag to indicate if tcp_sequence is valid, since } > it can legally be zero. } } Ack, will do. } } > We should also bail out if the sequence check fails, } > since it isn't possible for there to be another connection with the same } > src/srcport/dst/dstport, so there is no sense in continuing the search. } } That is was we do right ? } } First we check if src/dst ip address and port numbers match, if not we } bail out, so if we reach the above check we know these match, then we } check for tcp sequence number, if this doesn't match we bail out. If the src/dst addresses and port numbers don't match, we start the next iteration of the loop. If the sequence numbers don't match, we want to exit the loop. I believe the continue should be changed to a break. I'll pretty much be off the net until the new year, so I won't be able to perform any further reviews until then. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-net" in the body of the message