INET: fix incorrect "inet_sock->is_icsk" assignment

PATCH Applied Paul Moore 4 participants 12 messages Jan 4, 2007 – Jan 10, 2007 1 message not in the archive

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Paul Moore

author

2007-01-04 23:56 UTC #1 lore raw

PATCH 1/1 INET: fix incorrect "inet_sock->is_icsk" assignment

From: Paul Moore <[email protected]>

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 <[email protected]>

---
 net/ipv4/af_inet.c  |    2 +-
 net/ipv6/af_inet6.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

net/ipv4/af_inet.c +1 -1

Index: 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;

net/ipv6/af_inet6.c +1 -1

Index: 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

David Miller

maintainer Applied

2007-01-05 00:57 UTC #2 lore raw

From: "Paul Moore" <[email protected]>
Date: Thu, 04 Jan 2007 15:04:31 -0500

Quoted 9 lines

From: Paul Moore <[email protected]>
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 <[email protected]>

Applied, thanks a lot Paul.

Arnaldo Carvalho de Melo

2007-01-05 02:32 UTC #3 lore raw

Quoted 11 lines · David Miller

From: "Paul Moore" <[email protected]>
Date: Thu, 04 Jan 2007 15:04:31 -0500
Quoted 8 lines
From: Paul Moore <[email protected]>
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.

Quoted 3 lines

Quoted 1 line
Signed-off-by Paul Moore <[email protected]>
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

David Miller

maintainer

2007-01-05 05:42 UTC #4 lore raw

From: "Arnaldo Carvalho de Melo" <[email protected]>
Date: Fri, 5 Jan 2007 00:32:31 -0200

Quoted 2 lines

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 :-)

Jarek Poplawski

2007-01-08 13:23 UTC #5 lore raw

In reply to Paul Moore

On 04-01-2007 21:04, Paul Moore wrote:
...

net/ipv4/af_inet.c lookup_protocol: · line 308

Quoted 7 lines

+++ net-2.6.20_bugfix_2/net/ipv4/af_inet.c

net/ipv4/af_inet.c +1 -1

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

Paul Moore

author

2007-01-08 14:47 UTC #6 lore raw

Quoted 5 lines · Jarek Poplawski

Quoted 4 lines · Paul Moore
+++ net-2.6.20_bugfix_2/net/ipv4/af_inet.c
(quoted diff) +0 -0
@@ -305,7 +305,7 @@ lookup_protocol:
 		sk->sk_reuse = 1;
 

Quoted 8 lines

net/ipv4/af_inet.c lookup_protocol:
Quoted 4 lines
net/ipv4/af_inet.c +1 -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.

Signature · 2 lines

paul moore
linux security @ hp

Jarek Poplawski

2007-01-09 08:41 UTC #7 lore raw

On Mon, Jan 08, 2007 at 09:47:29AM -0500, Paul Moore wrote:
...

Quoted 1 line

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.

Quoted 2 lines

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.

Paul Moore

author

2007-01-09 14:46 UTC #8 lore raw

Quoted 3 lines · Jarek Poplawski

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

Signature · 2 lines

paul moore
linux security @ hp

Arnaldo Carvalho de Melo

2007-01-09 14:52 UTC #9 lore raw

Quoted 8 lines · Paul Moore

Quoted 3 lines · Jarek Poplawski
... 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

Jarek Poplawski

2007-01-10 09:59 UTC #10 lore raw

In reply to Paul Moore

Quoted 8 lines · Paul Moore

Quoted 3 lines · Jarek Poplawski
... 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.

Paul Moore

author

2007-01-10 13:14 UTC #11 lore raw

Quoted 6 lines · Jarek Poplawski

Quoted 5 lines · Paul Moore
Quoted 3 lines · Jarek Poplawski
... 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...

Quoted 7 lines

Quoted 4 lines
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 ;)

Signature · 2 lines

paul moore
linux security @ hp

Jarek Poplawski

2007-01-10 13:29 UTC #12 lore raw

Quoted 12 lines · Paul Moore

Quoted 11 lines · Jarek Poplawski
Quoted 9 lines · Paul Moore
Quoted 3 lines · Jarek Poplawski
... 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.

Quoted 6 lines

Quoted 2 lines
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.