netdev - Re: [net-next-2.6 PATCH v4 3/3] TCPCT part 1c: initial SYN exchange with SYNACK data

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] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 2 Nov 2009 14:57:43 +0200 (EET)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	William Allen Simpson <william.allen.simpson@...il.com>
cc:	Eric Dumazet <eric.dumazet@...il.com>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [net-next-2.6 PATCH v4 3/3] TCPCT part 1c: initial SYN exchange
 with SYNACK data

On Mon, 2 Nov 2009, William Allen Simpson wrote:

> Eric Dumazet wrote:
> > This part is really hard to review, and might be splitted ?
> > 
> > cleanups could be done in a cleanup patch only
> > 
> > Examples:
> > 
> > -	tmp_opt.mss_clamp = 536;
> > -	tmp_opt.user_mss  = tcp_sk(sk)->rx_opt.user_mss;
> > +	tmp_opt.mss_clamp = TCP_MIN_RCVMSS;
> > +	tmp_opt.user_mss  = tp->rx_opt.user_mss;
> > 
> > 
> > -	tp->mss_cache = 536;
> > +	tp->mss_cache = TCP_MIN_RCVMSS;
> > 
> Often hard to decide what's "cleanup" and what's essential.  I'll look at
> that again for the next round, but I've already split the original single
> patch into multiple parts.

Are you talking about particular case?!? ...You can safely split into even 
more parts if there are cleanups which is essential. ...We'll not stop you 
from doing that nor be angry if do that.

> > Also your tests are reversed, if you look at the existing coding style.
> > 
> I checked Documentation/CodingStyle, and that's not specified.  I've seen
> plenty of examples of modern security coding style around here.
> 
> As a long-time (25+ years) consultant and 30 years C programmer, I'm
> heedful of the project coding style, and had to endure many variants.
> 
> Where I'm working with others' code, you'll note that I keep the same
> style, no matter how ugly, as that makes patches easier to read.
> 
> 
> > Example :
> > 
> > +	/* TCP Cookie Transactions */
> > +	if (0 < sysctl_tcp_cookie_size) {
> > +		/* Default, cookies without s_data. */
> > +		tp->cookie_values =
> > +			kzalloc(sizeof(*tp->cookie_values),
> > +				sk->sk_allocation);
> > +		if (NULL != tp->cookie_values)
> > +			kref_init(&tp->cookie_values->kref);
> > +	}
> > 
> > should be ->
> > 
> > +	/* TCP Cookie Transactions */
> > +	if (sysctl_tcp_cookie_size > 0) {
> > +		/* Default, cookies without s_data. */
> > +		tp->cookie_values =
> > +			kzalloc(sizeof(*tp->cookie_values),
> > +				sk->sk_allocation);
> > +		if (tp->cookie_values != NULL)
> > +			kref_init(&tp->cookie_values->kref);
> > +	}
> > 
> And "tp->cookie_values != NULL" is egregiously poor C practice.  It's very
> hard for code review to ensure that didn't get truncated to "= NULL".  The
> important visual element is the NULL, not the variable name.
> 
> Also, avoid "!tp->cookie_values", as this is *not* a boolean.
> 
> When I'm adding new code, I use constant-to-the-left security coding style,
> as they teach in modern universities (lately also for PHP).  And this is a
> security extension, so a security style is particularly appropriate.
> 
> As in switch statements, constant-to-the-left makes the value obvious,
> especially in a series (and assists transforming if series into a switch).
> 
> For complex tests, this makes the code much more readable and easier to
> visually verify on code walk-through:
> 
> +	if (0 < tmp_opt.cookie_plus
> +	 && tmp_opt.saw_tstamp
> +	 && !tp->cookie_out_never
> +	 && (0 < sysctl_tcp_cookie_size
> +	  || (NULL != tp->cookie_values
> +	   && 0 < tp->cookie_values->cookie_desired))) {
> 
> Consistent use of security style would have obviated a lot of foolish >= 0
> tests that seem to be constantly in need of fixing.  It's a bad idea to
> depend on the compiler to catch non-executable code.

That kind of response certainly won't help you any. ...First, you said you 
adapt the current style but for some reason immediately start to say why 
you would careless about that principle. ...Also, telling that you have 
lots of experience here and there will not get you there either ;-).

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