[PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Paul Moore]

[-- Attachment #1: network-fix_icsk_assignment --]
[-- Type: text/plain, Size: 1544 bytes --]

From: Paul Moore <paul.moore@hp.com>

The inet_create() and inet6_create() functions incorrectly set the
inet_sock->is_icsk field.  Both functions assume that the is_icsk field is
large enough to hold at least a INET_PROTOSW_ICSK value when it is actually
only a single bit.  This patch corrects the assignment by doing a boolean
comparison whose result will safely fit into a single bit field.

Signed-off-by: Paul Moore <paul.moore@hp.com>
---
 net/ipv4/af_inet.c  |    2 +-
 net/ipv6/af_inet6.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Index: net-2.6.20_bugfix_2/net/ipv4/af_inet.c
===================================================================
--- net-2.6.20_bugfix_2.orig/net/ipv4/af_inet.c
+++ net-2.6.20_bugfix_2/net/ipv4/af_inet.c
@@ -305,7 +305,7 @@ lookup_protocol:
 		sk->sk_reuse = 1;
 
 	inet = inet_sk(sk);
-	inet->is_icsk = INET_PROTOSW_ICSK & answer_flags;
+	inet->is_icsk = (INET_PROTOSW_ICSK & answer_flags) == INET_PROTOSW_ICSK;
 
 	if (SOCK_RAW == sock->type) {
 		inet->num = protocol;
Index: net-2.6.20_bugfix_2/net/ipv6/af_inet6.c
===================================================================
--- net-2.6.20_bugfix_2.orig/net/ipv6/af_inet6.c
+++ net-2.6.20_bugfix_2/net/ipv6/af_inet6.c
@@ -171,7 +171,7 @@ lookup_protocol:
 		sk->sk_reuse = 1;
 
 	inet = inet_sk(sk);
-	inet->is_icsk = INET_PROTOSW_ICSK & answer_flags;
+	inet->is_icsk = (INET_PROTOSW_ICSK & answer_flags) == INET_PROTOSW_ICSK;
 
 	if (SOCK_RAW == sock->type) {
 		inet->num = protocol;

--
paul moore
linux security @ hp

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[David Miller]

From: "Paul Moore" <paul.moore@hp.com>
Date: Thu, 04 Jan 2007 15:04:31 -0500

> From: Paul Moore <paul.moore@hp.com>
> 
> The inet_create() and inet6_create() functions incorrectly set the
> inet_sock->is_icsk field.  Both functions assume that the is_icsk field is
> large enough to hold at least a INET_PROTOSW_ICSK value when it is actually
> only a single bit.  This patch corrects the assignment by doing a boolean
> comparison whose result will safely fit into a single bit field.
> 
> Signed-off-by: Paul Moore <paul.moore@hp.com>

Applied, thanks a lot Paul.

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Arnaldo Carvalho de Melo]

On 1/4/07, David Miller <davem@davemloft.net> wrote:
> From: "Paul Moore" <paul.moore@hp.com>
> Date: Thu, 04 Jan 2007 15:04:31 -0500
>
> > From: Paul Moore <paul.moore@hp.com>
> >
> > The inet_create() and inet6_create() functions incorrectly set the
> > inet_sock->is_icsk field.  Both functions assume that the is_icsk field is
> > large enough to hold at least a INET_PROTOSW_ICSK value when it is actually
> > only a single bit.  This patch corrects the assignment by doing a boolean
> > comparison whose result will safely fit into a single bit field.
> >
> > Signed-off-by: Paul Moore <paul.moore@hp.com>
>
> Applied, thanks a lot Paul.

Well spotted, gcc let me down on this one:

[acme@newtoy ~]$ cat a.c
#include <stdio.h>

struct foo { int a_bit:1; };

int main(void) {
        int trickme = 0x04;
        struct foo oof;

        oof.a_bit = trickme & 4;
        printf("%u\n", oof.a_bit);
}
[acme@newtoy ~]$ make a
cc     a.c   -o a
[acme@newtoy ~]$ ./a
0
[acme@newtoy ~]$

But...

[acme@newtoy ~]$ cat a.c
#include <stdio.h>

struct foo { int a_bit:1; };

int main(void) {
        struct foo oof;

        oof.a_bit = 0x04;
        printf("%u\n", oof.a_bit);
}
[acme@newtoy ~]$ make a
cc     a.c   -o a
a.c: In function 'main':
a.c:8: warning: overflow in implicit constant conversion
[acme@newtoy ~]$

I expected a warning since the and operation clearly could yield a
value that would overflow, just like in the constant case...

Anyway, thanks a lot Paul!

- Arnaldo

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[David Miller]

From: "Arnaldo Carvalho de Melo" <arnaldo.melo@gmail.com>
Date: Fri, 5 Jan 2007 00:32:31 -0200

> I expected a warning since the and operation clearly could yield a
> value that would overflow, just like in the constant case...

It sounds stupid, but once you introduce variables and not
everything is constance, GCC currently can't make the analysis.

It's an interesting class of bugs to detect automatically, for
sure :-)

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Jarek Poplawski]

On 04-01-2007 21:04, Paul Moore wrote:
...
> +++ net-2.6.20_bugfix_2/net/ipv4/af_inet.c
> @@ -305,7 +305,7 @@ lookup_protocol:
>  		sk->sk_reuse = 1;
>  
>  	inet = inet_sk(sk);
> -	inet->is_icsk = INET_PROTOSW_ICSK & answer_flags;
> +	inet->is_icsk = (INET_PROTOSW_ICSK & answer_flags) == INET_PROTOSW_ICSK;

Isn't this more readable like this?:
 
inet->is_icsk = (INET_PROTOSW_ICSK & answer_flags) != 0;

Regards,
Jarek P.

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Paul Moore]

On Monday, January 8 2007 8:25 am, Jarek Poplawski wrote:
> On 04-01-2007 21:04, Paul Moore wrote:
> > +++ net-2.6.20_bugfix_2/net/ipv4/af_inet.c
> > @@ -305,7 +305,7 @@ lookup_protocol:
> >  		sk->sk_reuse = 1;
> >
> >  	inet = inet_sk(sk);
> > -	inet->is_icsk = INET_PROTOSW_ICSK & answer_flags;
> > +	inet->is_icsk = (INET_PROTOSW_ICSK & answer_flags) ==
> > INET_PROTOSW_ICSK;
>
> Isn't this more readable like this?:
>
> inet->is_icsk = (INET_PROTOSW_ICSK & answer_flags) != 0;

I guess it all depends on who is reading it ;)  Personally, I don't care too 
much either way as long as it is fixed.

-- 
paul moore
linux security @ hp

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Jarek Poplawski]

On Mon, Jan 08, 2007 at 09:47:29AM -0500, Paul Moore wrote:
...
> I guess it all depends on who is reading it ;)

Sure! I only had a feeling your way is maybe slightly
less often used so I wanted some opinion.

> Personally, I don't care too 
> much either way as long as it is fixed.

Yes, you've done great work, no doubt. But if you
consider this code will probably become classical
and will be read, quoted and teached next 1000
years, then the style could matter...

Cheers,
Jarek P. 

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Paul Moore]

On Tuesday 09 January 2007 3:43 am, Jarek Poplawski wrote:
> ... But if you consider this code will probably become classical
> and will be read, quoted and teached next 1000 years, then the style
> could matter... 

This from the guy who believes "Justin Timberlake rocks!" ;)

All right, you convinced me, I'll send out a patch to the patch later today.

-- 
paul moore
linux security @ hp

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Arnaldo Carvalho de Melo]

On 1/9/07, Paul Moore <paul.moore@hp.com> wrote:
> On Tuesday 09 January 2007 3:43 am, Jarek Poplawski wrote:
> > ... But if you consider this code will probably become classical
> > and will be read, quoted and teached next 1000 years, then the style
> > could matter...
>
> This from the guy who believes "Justin Timberlake rocks!" ;)
>
> All right, you convinced me, I'll send out a patch to the patch later today.

/me grins

I see I don't have to worry that much with style nitpicking, keep it up!

- Arnaldo

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Jarek Poplawski]

On Tue, Jan 09, 2007 at 09:26:46AM -0500, Paul Moore wrote:
> On Tuesday 09 January 2007 3:43 am, Jarek Poplawski wrote:
> > ... But if you consider this code will probably become classical
> > and will be read, quoted and teached next 1000 years, then the style
> > could matter... 
> 
> This from the guy who believes "Justin Timberlake rocks!" ;)
> 
> All right, you convinced me, I'll send out a patch to the patch later today.

I'm terribly sorry, I didn't expected any messing now!
I rather thought about some preferences for the future.

Justin really rocks (now), in particular, as an example 
to the popular HOWTO. Jennifer Lopez, Snoop Dogg or 50
Cent as well. But for an IEEE document: Pink Floyd, Led
Zeppelin or Jimi Hendrix would be more appropriate,
probably, and obviously they also rock more!

Cheers,
Jarek P. 

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Paul Moore]

On Wednesday 10 January 2007 5:01 am, Jarek Poplawski wrote:
> On Tue, Jan 09, 2007 at 09:26:46AM -0500, Paul Moore wrote:
> > On Tuesday 09 January 2007 3:43 am, Jarek Poplawski wrote:
> > > ... But if you consider this code will probably become classical
> > > and will be read, quoted and teached next 1000 years, then the style
> > > could matter...
> >
> > This from the guy who believes "Justin Timberlake rocks!" ;)
> >
> > All right, you convinced me, I'll send out a patch to the patch later
> > today.
>
> I'm terribly sorry, I didn't expected any messing now!
> I rather thought about some preferences for the future.

No need to apologize, it was a trivial fix and I figured I might as well take 
care of now ... however, some of your taste in music does require an 
apology ;)

-- 
paul moore
linux security @ hp

Re: [PATCH] INET: fix incorrect "inet_sock->is_icsk" assignment

[Jarek Poplawski]

On Wed, Jan 10, 2007 at 08:13:57AM -0500, Paul Moore wrote:
> On Wednesday 10 January 2007 5:01 am, Jarek Poplawski wrote:
> > On Tue, Jan 09, 2007 at 09:26:46AM -0500, Paul Moore wrote:
> > > On Tuesday 09 January 2007 3:43 am, Jarek Poplawski wrote:
> > > > ... But if you consider this code will probably become classical
> > > > and will be read, quoted and teached next 1000 years, then the style
> > > > could matter...
> > >
> > > This from the guy who believes "Justin Timberlake rocks!" ;)
> > >
> > > All right, you convinced me, I'll send out a patch to the patch later
> > > today.
> >
> > I'm terribly sorry, I didn't expected any messing now!
> > I rather thought about some preferences for the future.
> 
> No need to apologize, it was a trivial fix and I figured I might as well take 
> care of now ... however, some of your taste in music does require an 
> apology ;)

All right, you convinced me too. I change 50 Cent to Busta Rhymes!

Jarek P.