diff mbox

[CVE-2011-1593,Hardy,1/2] next_pidmap: fix overflow condition, CVE-2011-1593

Message ID 1305753408-6848-2-git-send-email-herton.krzesinski@canonical.com
State New
Headers show

Commit Message

Herton Ronaldo Krzesinski May 18, 2011, 9:16 p.m. UTC 
From: Linus Torvalds <torvalds@linux-foundation.org>

CVE-2011-1593

BugLink: https://bugs.launchpad.net/bugs/784727

Released until now with stable versions 2.6.27.59, 2.6.32.39, 2.6.33.12,
2.6.35.13, 2.6.38.4

next_pidmap() just quietly accepted whatever 'last' pid that was passed
in, which is not all that safe when one of the users is /proc.

Admittedly the proc code should do some sanity checking on the range
(and that will be the next commit), but that doesn't mean that the
helper functions should just do that pidmap pointer arithmetic without
checking the range of its arguments.

So clamp 'last' to PID_MAX_LIMIT.  The fact that we then do "last+1"
doesn't really matter, the for-loop does check against the end of the
pidmap array properly (it's only the actual pointer arithmetic overflow
case we need to worry about, and going one bit beyond isn't going to
overflow).

[ Use PID_MAX_LIMIT rather than pid_max as per Eric Biederman ]

Reported-by: Tavis Ormandy <taviso@cmpxchg8b.com>
Analyzed-by: Robert Święcki <robert@swiecki.net>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Pavel Emelyanov <xemul@openvz.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(backported from commit c78193e9c7bcbf25b8237ad0dec82f805c4ea69b upstream)
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
---
 .../openvz/patchset/0001-2.6.24-ovz002.patch       |    2 +-
 kernel/pid.c                                       |    5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Brad Figg May 18, 2011, 10:12 p.m. UTC | #1 
On 05/18/2011 02:16 PM, Herton Ronaldo Krzesinski wrote:
> From: Linus Torvalds<torvalds@linux-foundation.org>
>
> CVE-2011-1593
>
> BugLink: https://bugs.launchpad.net/bugs/784727
>
> Released until now with stable versions 2.6.27.59, 2.6.32.39, 2.6.33.12,
> 2.6.35.13, 2.6.38.4
>
> next_pidmap() just quietly accepted whatever 'last' pid that was passed
> in, which is not all that safe when one of the users is /proc.
>
> Admittedly the proc code should do some sanity checking on the range
> (and that will be the next commit), but that doesn't mean that the
> helper functions should just do that pidmap pointer arithmetic without
> checking the range of its arguments.
>
> So clamp 'last' to PID_MAX_LIMIT.  The fact that we then do "last+1"
> doesn't really matter, the for-loop does check against the end of the
> pidmap array properly (it's only the actual pointer arithmetic overflow
> case we need to worry about, and going one bit beyond isn't going to
> overflow).
>
> [ Use PID_MAX_LIMIT rather than pid_max as per Eric Biederman ]
>
> Reported-by: Tavis Ormandy<taviso@cmpxchg8b.com>
> Analyzed-by: Robert Święcki<robert@swiecki.net>
> Cc: Eric W. Biederman<ebiederm@xmission.com>
> Cc: Pavel Emelyanov<xemul@openvz.org>
> Signed-off-by: Linus Torvalds<torvalds@linux-foundation.org>
> (backported from commit c78193e9c7bcbf25b8237ad0dec82f805c4ea69b upstream)
> Signed-off-by: Herton Ronaldo Krzesinski<herton.krzesinski@canonical.com>
> ---
>   .../openvz/patchset/0001-2.6.24-ovz002.patch       |    2 +-
>   kernel/pid.c                                       |    5 ++++-
>   2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> index 729b278..6a8a613 100644
> --- a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> +++ b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> @@ -62556,7 +62556,7 @@ Index: kernel/kernel/pid.c
>   +	return pid;
>   +}
>   +
> - static int next_pidmap(struct pid_namespace *pid_ns, int last)
> + static int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>    {
>    	int offset;
>   @@ -198,6 +231,7 @@
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f815455..29f0ac0 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -181,11 +181,14 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
>   	return -1;
>   }
>
> -static int next_pidmap(struct pid_namespace *pid_ns, int last)
> +static int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>   {
>   	int offset;
>   	struct pidmap *map, *end;
>
> +	if (last>= PID_MAX_LIMIT)
> +		return -1;
> +
>   	offset = (last + 1)&  BITS_PER_PAGE_MASK;
>   	map =&pid_ns->pidmap[(last + 1)/BITS_PER_PAGE];
>   	end =&pid_ns->pidmap[PIDMAP_ENTRIES];

Acked-by: Brad Figg <brad.figg@canonical.com>
John Johansen May 19, 2011, 12:19 a.m. UTC | #2 
On 05/18/2011 02:16 PM, Herton Ronaldo Krzesinski wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> CVE-2011-1593
> 
> BugLink: https://bugs.launchpad.net/bugs/784727
> 
> Released until now with stable versions 2.6.27.59, 2.6.32.39, 2.6.33.12,
> 2.6.35.13, 2.6.38.4
> 
> next_pidmap() just quietly accepted whatever 'last' pid that was passed
> in, which is not all that safe when one of the users is /proc.
> 
> Admittedly the proc code should do some sanity checking on the range
> (and that will be the next commit), but that doesn't mean that the
> helper functions should just do that pidmap pointer arithmetic without
> checking the range of its arguments.
> 
> So clamp 'last' to PID_MAX_LIMIT.  The fact that we then do "last+1"
> doesn't really matter, the for-loop does check against the end of the
> pidmap array properly (it's only the actual pointer arithmetic overflow
> case we need to worry about, and going one bit beyond isn't going to
> overflow).
> 
> [ Use PID_MAX_LIMIT rather than pid_max as per Eric Biederman ]
> 
> Reported-by: Tavis Ormandy <taviso@cmpxchg8b.com>
> Analyzed-by: Robert Święcki <robert@swiecki.net>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Pavel Emelyanov <xemul@openvz.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (backported from commit c78193e9c7bcbf25b8237ad0dec82f805c4ea69b upstream)
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>

Acked-by: John Johansen <john.johansen@canonical.com>

> ---
>  .../openvz/patchset/0001-2.6.24-ovz002.patch       |    2 +-
>  kernel/pid.c                                       |    5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> index 729b278..6a8a613 100644
> --- a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> +++ b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> @@ -62556,7 +62556,7 @@ Index: kernel/kernel/pid.c
>  +	return pid;
>  +}
>  +
> - static int next_pidmap(struct pid_namespace *pid_ns, int last)
> + static int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>   {
>   	int offset;
>  @@ -198,6 +231,7 @@
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f815455..29f0ac0 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -181,11 +181,14 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
>  	return -1;
>  }
>  
> -static int next_pidmap(struct pid_namespace *pid_ns, int last)
> +static int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>  {
>  	int offset;
>  	struct pidmap *map, *end;
>  
> +	if (last >= PID_MAX_LIMIT)
> +		return -1;
> +
>  	offset = (last + 1) & BITS_PER_PAGE_MASK;
>  	map = &pid_ns->pidmap[(last + 1)/BITS_PER_PAGE];
>  	end = &pid_ns->pidmap[PIDMAP_ENTRIES];
Steve Conklin May 24, 2011, 9:30 p.m. UTC | #3 
Applied

On Wed, 2011-05-18 at 18:16 -0300, Herton Ronaldo Krzesinski wrote:
> From: Linus Torvalds <torvalds@linux-foundation.org>
> 
> CVE-2011-1593
> 
> BugLink: https://bugs.launchpad.net/bugs/784727
> 
> Released until now with stable versions 2.6.27.59, 2.6.32.39, 2.6.33.12,
> 2.6.35.13, 2.6.38.4
> 
> next_pidmap() just quietly accepted whatever 'last' pid that was passed
> in, which is not all that safe when one of the users is /proc.
> 
> Admittedly the proc code should do some sanity checking on the range
> (and that will be the next commit), but that doesn't mean that the
> helper functions should just do that pidmap pointer arithmetic without
> checking the range of its arguments.
> 
> So clamp 'last' to PID_MAX_LIMIT.  The fact that we then do "last+1"
> doesn't really matter, the for-loop does check against the end of the
> pidmap array properly (it's only the actual pointer arithmetic overflow
> case we need to worry about, and going one bit beyond isn't going to
> overflow).
> 
> [ Use PID_MAX_LIMIT rather than pid_max as per Eric Biederman ]
> 
> Reported-by: Tavis Ormandy <taviso@cmpxchg8b.com>
> Analyzed-by: Robert Święcki <robert@swiecki.net>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Pavel Emelyanov <xemul@openvz.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> (backported from commit c78193e9c7bcbf25b8237ad0dec82f805c4ea69b upstream)
> Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
> ---
>  .../openvz/patchset/0001-2.6.24-ovz002.patch       |    2 +-
>  kernel/pid.c                                       |    5 ++++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> index 729b278..6a8a613 100644
> --- a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> +++ b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
> @@ -62556,7 +62556,7 @@ Index: kernel/kernel/pid.c
>  +	return pid;
>  +}
>  +
> - static int next_pidmap(struct pid_namespace *pid_ns, int last)
> + static int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>   {
>   	int offset;
>  @@ -198,6 +231,7 @@
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f815455..29f0ac0 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -181,11 +181,14 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
>  	return -1;
>  }
>  
> -static int next_pidmap(struct pid_namespace *pid_ns, int last)
> +static int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>  {
>  	int offset;
>  	struct pidmap *map, *end;
>  
> +	if (last >= PID_MAX_LIMIT)
> +		return -1;
> +
>  	offset = (last + 1) & BITS_PER_PAGE_MASK;
>  	map = &pid_ns->pidmap[(last + 1)/BITS_PER_PAGE];
>  	end = &pid_ns->pidmap[PIDMAP_ENTRIES];
> -- 
> 1.7.0.4
> 
>
diff mbox

Patch

diff --git a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
index 729b278..6a8a613 100644
--- a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
+++ b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
@@ -62556,7 +62556,7 @@  Index: kernel/kernel/pid.c
 +	return pid;
 +}
 +
- static int next_pidmap(struct pid_namespace *pid_ns, int last)
+ static int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
  {
  	int offset;
 @@ -198,6 +231,7 @@
diff --git a/kernel/pid.c b/kernel/pid.c
index f815455..29f0ac0 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -181,11 +181,14 @@  static int alloc_pidmap(struct pid_namespace *pid_ns)
 	return -1;
 }
 
-static int next_pidmap(struct pid_namespace *pid_ns, int last)
+static int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
 {
 	int offset;
 	struct pidmap *map, *end;
 
+	if (last >= PID_MAX_LIMIT)
+		return -1;
+
 	offset = (last + 1) & BITS_PER_PAGE_MASK;
 	map = &pid_ns->pidmap[(last + 1)/BITS_PER_PAGE];
 	end = &pid_ns->pidmap[PIDMAP_ENTRIES];