[PATCH] netlink: fix overrun in attribute iteration

[PATCH] netlink: fix overrun in attribute iteration

[Vegard Nossum]

>From ca63375e8ed91d73d0c2abd1cb64a8b022ce2af8 Mon Sep 17 00:00:00 2001
From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Thu, 11 Sep 2008 22:37:13 +0200
Subject: [PATCH] netlink: fix overrun in attribute iteration

kmemcheck reported this:

  kmemcheck: Caught 16-bit read from uninitialized memory (f6c1ba30)
  0500110001508abf050010000500000002017300140000006f72672e66726565
   i i i i i i i i i i i i i u u u u u u u u u u u u u u u u u u u
                                   ^

  Pid: 3462, comm: wpa_supplicant Not tainted (2.6.27-rc3-00054-g6397ab9-dirty #13)
  EIP: 0060:[<c05de64a>] EFLAGS: 00010296 CPU: 0
  EIP is at nla_parse+0x5a/0xf0
  EAX: 00000008 EBX: fffffffd ECX: c06f16c0 EDX: 00000005
  ESI: 00000010 EDI: f6c1ba30 EBP: f6367c6c ESP: c0a11e88
   DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
  CR0: 8005003b CR2: f781cc84 CR3: 3632f000 CR4: 000006d0
  DR0: c0ead9bc DR1: 00000000 DR2: 00000000 DR3: 00000000
  DR6: ffff4ff0 DR7: 00000400
   [<c05d4b23>] rtnl_setlink+0x63/0x130
   [<c05d5f75>] rtnetlink_rcv_msg+0x165/0x200
   [<c05ddf66>] netlink_rcv_skb+0x76/0xa0
   [<c05d5dfe>] rtnetlink_rcv+0x1e/0x30
   [<c05dda21>] netlink_unicast+0x281/0x290
   [<c05ddbe9>] netlink_sendmsg+0x1b9/0x2b0
   [<c05beef2>] sock_sendmsg+0xd2/0x100
   [<c05bf945>] sys_sendto+0xa5/0xd0
   [<c05bf9a6>] sys_send+0x36/0x40
   [<c05c03d6>] sys_socketcall+0x1e6/0x2c0
   [<c020353b>] sysenter_do_call+0x12/0x3f
   [<ffffffff>] 0xffffffff

This is the line in nla_ok():

  /**
   * nla_ok - check if the netlink attribute fits into the remaining bytes
   * @nla: netlink attribute
   * @remaining: number of bytes remaining in attribute stream
   */
  static inline int nla_ok(const struct nlattr *nla, int remaining)
  {
          return remaining >= sizeof(*nla) &&
                 nla->nla_len >= sizeof(*nla) &&
                 nla->nla_len <= remaining;
  }

It turns out that remaining can become negative due to alignment in
nla_next(). But GCC promotes "remaining" to unsigned in the test
against sizeof(*nla) above. Therefore the test succeeds, and the
nla_for_each_attr() may access memory outside the received buffer.

A short example illustrating this point is here:

  #include <stdio.h>

  main(void)
  {
          printf("%d\n", -1 >= sizeof(int));
  }

...which prints "1".

This patch adds a cast in front of the sizeof so that GCC will make
a signed comparison and fix the illegal memory dereference. With the
patch applied, there is no kmemcheck report.

Cc: Thomas Graf <tgraf@suug.ch>
Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>
---
 include/net/netlink.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 18024b8..208fe5a 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -702,7 +702,7 @@ static inline int nla_len(const struct nlattr *nla)
  */
 static inline int nla_ok(const struct nlattr *nla, int remaining)
 {
-	return remaining >= sizeof(*nla) &&
+	return remaining >= (int) sizeof(*nla) &&
 	       nla->nla_len >= sizeof(*nla) &&
 	       nla->nla_len <= remaining;
 }
-- 
1.5.5.1

Re: [PATCH] netlink: fix overrun in attribute iteration

[David Miller]

From: Vegard Nossum <vegard.nossum@gmail.com>
Date: Thu, 11 Sep 2008 22:59:33 +0200

> A short example illustrating this point is here:
> 
>   #include <stdio.h>
> 
>   main(void)
>   {
>           printf("%d\n", -1 >= sizeof(int));
>   }
> 
> ...which prints "1".

Someone should print that out on a huge poster, it's a good
example of why C promotion rules suck :)

> This patch adds a cast in front of the sizeof so that GCC will make
> a signed comparison and fix the illegal memory dereference. With the
> patch applied, there is no kmemcheck report.
> 
> Cc: Thomas Graf <tgraf@suug.ch>
> Signed-off-by: Vegard Nossum <vegard.nossum@gmail.com>

Thomas, please review.

> diff --git a/include/net/netlink.h b/include/net/netlink.h
> index 18024b8..208fe5a 100644
> --- a/include/net/netlink.h
> +++ b/include/net/netlink.h
> @@ -702,7 +702,7 @@ static inline int nla_len(const struct nlattr *nla)
>   */
>  static inline int nla_ok(const struct nlattr *nla, int remaining)
>  {
> -	return remaining >= sizeof(*nla) &&
> +	return remaining >= (int) sizeof(*nla) &&
>  	       nla->nla_len >= sizeof(*nla) &&
>  	       nla->nla_len <= remaining;
>  }
> -- 
> 1.5.5.1
> 

Re: [PATCH] netlink: fix overrun in attribute iteration

[Andrew Morton]

On Thu, 11 Sep 2008 22:59:33 +0200
Vegard Nossum <vegard.nossum@gmail.com> wrote:

>   #include <stdio.h>
> 
>   main(void)
>   {
>           printf("%d\n", -1 >= sizeof(int));
>   }
> 

akpm:/home/akpm> gcc -W t.c
t.c: In function 'main':
t.c:5: warning: comparison between signed and unsigned

Make of that what you will :)

Re: [PATCH] netlink: fix overrun in attribute iteration

[Thomas Graf]

* David Miller <davem@davemloft.net> 2008-09-11 15:04
> From: Vegard Nossum <vegard.nossum@gmail.com>
> Thomas, please review.
> 
> > diff --git a/include/net/netlink.h b/include/net/netlink.h
> > index 18024b8..208fe5a 100644
> > --- a/include/net/netlink.h
> > +++ b/include/net/netlink.h
> > @@ -702,7 +702,7 @@ static inline int nla_len(const struct nlattr *nla)
> >   */
> >  static inline int nla_ok(const struct nlattr *nla, int remaining)
> >  {
> > -	return remaining >= sizeof(*nla) &&
> > +	return remaining >= (int) sizeof(*nla) &&
> >  	       nla->nla_len >= sizeof(*nla) &&
> >  	       nla->nla_len <= remaining;
> >  }

Very nice catch, would never have thought of that.

Re: [PATCH] netlink: fix overrun in attribute iteration

[David Miller]

From: Thomas Graf <tgraf@suug.ch>
Date: Fri, 12 Sep 2008 02:35:23 +0200

> * David Miller <davem@davemloft.net> 2008-09-11 15:04
> > From: Vegard Nossum <vegard.nossum@gmail.com>
> > Thomas, please review.
> > 
> > > diff --git a/include/net/netlink.h b/include/net/netlink.h
> > > index 18024b8..208fe5a 100644
> > > --- a/include/net/netlink.h
> > > +++ b/include/net/netlink.h
> > > @@ -702,7 +702,7 @@ static inline int nla_len(const struct nlattr *nla)
> > >   */
> > >  static inline int nla_ok(const struct nlattr *nla, int remaining)
> > >  {
> > > -	return remaining >= sizeof(*nla) &&
> > > +	return remaining >= (int) sizeof(*nla) &&
> > >  	       nla->nla_len >= sizeof(*nla) &&
> > >  	       nla->nla_len <= remaining;
> > >  }
> 
> Very nice catch, would never have thought of that.

Applied, thanks everyone.

Re: [PATCH] netlink: fix overrun in attribute iteration

[David Wagner]

Vegard Nossum  wrote:
>  /**
>   * nla_ok - check if the netlink attribute fits into the remaining bytes
>   * @nla: netlink attribute
>   * @remaining: number of bytes remaining in attribute stream
>   */
>  static inline int nla_ok(const struct nlattr *nla, int remaining)
>  {
>          return remaining >= sizeof(*nla) &&
>                 nla->nla_len >= sizeof(*nla) &&
>                 nla->nla_len <= remaining;
>  }

If 'remaining' had been declared to be of type size_t, this would
not have happened.

Guideline for secure programming: length values (and counts of bytes)
should be declared as size_t, where possible.

This guideline eliminates many signed/unsigned bugs.  If one also
carefully avoids overflow (wraparound), other integer overflow bugs
are avoided as well.

The code above violates the guideline so we shouldn't be terribly
surprised if it happens to contain signed/unsigned vulnerabilities.

Re: [PATCH] netlink: fix overrun in attribute iteration

[Vegard Nossum]

On Fri, Sep 12, 2008 at 1:52 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 11 Sep 2008 22:59:33 +0200
> Vegard Nossum <vegard.nossum@gmail.com> wrote:
>
>>   #include <stdio.h>
>>
>>   main(void)
>>   {
>>           printf("%d\n", -1 >= sizeof(int));
>>   }
>>
>
> akpm:/home/akpm> gcc -W t.c
> t.c: In function 'main':
> t.c:5: warning: comparison between signed and unsigned
>
> Make of that what you will :)

It doesn't show up with -Wall and the kernel isn't compiled with -W
(aka. -Wextra) as far as I can see. Should it be turned on?


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH] netlink: fix overrun in attribute iteration

[Vegard Nossum]

On Fri, Sep 12, 2008 at 5:51 AM, David Wagner <daw@cs.berkeley.edu> wrote:
> Vegard Nossum  wrote:
>>  /**
>>   * nla_ok - check if the netlink attribute fits into the remaining bytes
>>   * @nla: netlink attribute
>>   * @remaining: number of bytes remaining in attribute stream
>>   */
>>  static inline int nla_ok(const struct nlattr *nla, int remaining)
>>  {
>>          return remaining >= sizeof(*nla) &&
>>                 nla->nla_len >= sizeof(*nla) &&
>>                 nla->nla_len <= remaining;
>>  }
>
> If 'remaining' had been declared to be of type size_t, this would
> not have happened.

Hm. Yes, it would!

The problem here is that "remaining" can legitimately contain negative
values (see the pointer advancement in nla_next()). And size_t can't
hold negative values.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: [PATCH] netlink: fix overrun in attribute iteration

[Andrew Morton]

On Fri, 12 Sep 2008 07:42:36 +0200 "Vegard Nossum" <vegard.nossum@gmail.com> wrote:

> On Fri, Sep 12, 2008 at 1:52 AM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Thu, 11 Sep 2008 22:59:33 +0200
> > Vegard Nossum <vegard.nossum@gmail.com> wrote:
> >
> >>   #include <stdio.h>
> >>
> >>   main(void)
> >>   {
> >>           printf("%d\n", -1 >= sizeof(int));
> >>   }
> >>
> >
> > akpm:/home/akpm> gcc -W t.c
> > t.c: In function 'main':
> > t.c:5: warning: comparison between signed and unsigned
> >
> > Make of that what you will :)
> 
> It doesn't show up with -Wall and the kernel isn't compiled with -W
> (aka. -Wextra) as far as I can see. Should it be turned on?
> 

Last time I turned on -W, a full kernel build emitted nearly 10MB of
warnings.

Alas, some of them are useful, as we see here.  They can be turned on
piecemeal - this one is -Wsign-compare, I think.

I think it would be good if owners of particular parts of the kernel
were to occasionally build their stuff with -W and spend half an hour
contemplating the result.  Ditto `make C=1', to see what sparse thinks.