Re: [PATCH] proc: pid/status: show all supplementary groups

2012-11-09 Thread Kees Cook
On Fri, Nov 9, 2012 at 5:31 AM, Artem Bityutskiy  wrote:
> From: Artem Bityutskiy 
>
> We display a list of supplementary group for each process in the
> /proc//status. However, we show only the first 32 groups, not all of 
> them.
>
> Although this is rare, but sometimes processes do have more than 32
> supplementary groups, and this kernel limitation breaks user-space apps
> that rely on the group list in /proc//status.
>
> Number 32 comes from the internal NGROUPS_SMALL macro which defines the
> length for the internal kernel "small" groups buffer. There is no apparent
> reason to limit to this value.
>
> This patch removes the 32 groups printing limit.
>
> The Linux kernel limits the amount of supplementary groups by NGROUPS_MAX,
> which is currently set to 65536. And this is the maximum count of groups we
> may possibly print.
>
> Signed-off-by: Artem Bityutskiy 

Acked-by: Kees Cook 

> Cc: sta...@vger.kernel.org
> ---
>  fs/proc/array.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> NOTE: I consider this to be a bug which breaks user-space, so I add -stable.

I'm not sure if this will fly since it's been broken for a very long
time, but it's a tiny change.

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] proc: pid/status: show all supplementary groups

2012-11-09 Thread Serge Hallyn
Quoting Artem Bityutskiy (dedeki...@gmail.com):
> From: Artem Bityutskiy 
> 
> We display a list of supplementary group for each process in the
> /proc//status. However, we show only the first 32 groups, not all of 
> them.
> 
> Although this is rare, but sometimes processes do have more than 32
> supplementary groups, and this kernel limitation breaks user-space apps
> that rely on the group list in /proc//status.
> 
> Number 32 comes from the internal NGROUPS_SMALL macro which defines the
> length for the internal kernel "small" groups buffer. There is no apparent
> reason to limit to this value.
> 
> This patch removes the 32 groups printing limit.
> 
> The Linux kernel limits the amount of supplementary groups by NGROUPS_MAX,
> which is currently set to 65536. And this is the maximum count of groups we
> may possibly print.
> 
> Signed-off-by: Artem Bityutskiy 

The 'min' is older than git history, but at that dawn of time the code
was just sprintf()ing into a large buffer.

I don't *really* see a problem with this, though if someone did have 1000
groups /proc/$$/status would be sort of annoying to read.  So on the one 
hand adding a '...' in /proc/self/status after 32, and adding a /proc/$$/creds
file seems more pleasant, but then you get into the whole adding files to
/proc kerfuffle, so...

Acked-by: Serge E. Hallyn 

> Cc: sta...@vger.kernel.org
> ---
>  fs/proc/array.c |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> NOTE: I consider this to be a bug which breaks user-space, so I add -stable.
> 
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index c1c207c..bd31e02 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -212,7 +212,7 @@ static inline void task_state(struct seq_file *m, struct 
> pid_namespace *ns,
>   group_info = cred->group_info;
>   task_unlock(p);
>  
> - for (g = 0; g < min(group_info->ngroups, NGROUPS_SMALL); g++)
> + for (g = 0; g < group_info->ngroups; g++)
>   seq_printf(m, "%d ",
>  from_kgid_munged(user_ns, GROUP_AT(group_info, g)));
>   put_cred(cred);
> -- 
> 1.7.7.6
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] proc: pid/status: show all supplementary groups

2012-11-09 Thread Artem Bityutskiy
From: Artem Bityutskiy 

We display a list of supplementary group for each process in the
/proc//status. However, we show only the first 32 groups, not all of them.

Although this is rare, but sometimes processes do have more than 32
supplementary groups, and this kernel limitation breaks user-space apps
that rely on the group list in /proc//status.

Number 32 comes from the internal NGROUPS_SMALL macro which defines the
length for the internal kernel "small" groups buffer. There is no apparent
reason to limit to this value.

This patch removes the 32 groups printing limit.

The Linux kernel limits the amount of supplementary groups by NGROUPS_MAX,
which is currently set to 65536. And this is the maximum count of groups we
may possibly print.

Signed-off-by: Artem Bityutskiy 
Cc: sta...@vger.kernel.org
---
 fs/proc/array.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

NOTE: I consider this to be a bug which breaks user-space, so I add -stable.

diff --git a/fs/proc/array.c b/fs/proc/array.c
index c1c207c..bd31e02 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -212,7 +212,7 @@ static inline void task_state(struct seq_file *m, struct 
pid_namespace *ns,
group_info = cred->group_info;
task_unlock(p);
 
-   for (g = 0; g < min(group_info->ngroups, NGROUPS_SMALL); g++)
+   for (g = 0; g < group_info->ngroups; g++)
seq_printf(m, "%d ",
   from_kgid_munged(user_ns, GROUP_AT(group_info, g)));
put_cred(cred);
-- 
1.7.7.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


[PATCH] proc: pid/status: show all supplementary groups

2012-11-09 Thread Artem Bityutskiy
From: Artem Bityutskiy artem.bityuts...@linux.intel.com

We display a list of supplementary group for each process in the
/proc/pid/status. However, we show only the first 32 groups, not all of them.

Although this is rare, but sometimes processes do have more than 32
supplementary groups, and this kernel limitation breaks user-space apps
that rely on the group list in /proc/pid/status.

Number 32 comes from the internal NGROUPS_SMALL macro which defines the
length for the internal kernel small groups buffer. There is no apparent
reason to limit to this value.

This patch removes the 32 groups printing limit.

The Linux kernel limits the amount of supplementary groups by NGROUPS_MAX,
which is currently set to 65536. And this is the maximum count of groups we
may possibly print.

Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com
Cc: sta...@vger.kernel.org
---
 fs/proc/array.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

NOTE: I consider this to be a bug which breaks user-space, so I add -stable.

diff --git a/fs/proc/array.c b/fs/proc/array.c
index c1c207c..bd31e02 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -212,7 +212,7 @@ static inline void task_state(struct seq_file *m, struct 
pid_namespace *ns,
group_info = cred-group_info;
task_unlock(p);
 
-   for (g = 0; g  min(group_info-ngroups, NGROUPS_SMALL); g++)
+   for (g = 0; g  group_info-ngroups; g++)
seq_printf(m, %d ,
   from_kgid_munged(user_ns, GROUP_AT(group_info, g)));
put_cred(cred);
-- 
1.7.7.6

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] proc: pid/status: show all supplementary groups

2012-11-09 Thread Serge Hallyn
Quoting Artem Bityutskiy (dedeki...@gmail.com):
 From: Artem Bityutskiy artem.bityuts...@linux.intel.com
 
 We display a list of supplementary group for each process in the
 /proc/pid/status. However, we show only the first 32 groups, not all of 
 them.
 
 Although this is rare, but sometimes processes do have more than 32
 supplementary groups, and this kernel limitation breaks user-space apps
 that rely on the group list in /proc/pid/status.
 
 Number 32 comes from the internal NGROUPS_SMALL macro which defines the
 length for the internal kernel small groups buffer. There is no apparent
 reason to limit to this value.
 
 This patch removes the 32 groups printing limit.
 
 The Linux kernel limits the amount of supplementary groups by NGROUPS_MAX,
 which is currently set to 65536. And this is the maximum count of groups we
 may possibly print.
 
 Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com

The 'min' is older than git history, but at that dawn of time the code
was just sprintf()ing into a large buffer.

I don't *really* see a problem with this, though if someone did have 1000
groups /proc/$$/status would be sort of annoying to read.  So on the one 
hand adding a '...' in /proc/self/status after 32, and adding a /proc/$$/creds
file seems more pleasant, but then you get into the whole adding files to
/proc kerfuffle, so...

Acked-by: Serge E. Hallyn serge.hal...@ubuntu.com

 Cc: sta...@vger.kernel.org
 ---
  fs/proc/array.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 NOTE: I consider this to be a bug which breaks user-space, so I add -stable.
 
 diff --git a/fs/proc/array.c b/fs/proc/array.c
 index c1c207c..bd31e02 100644
 --- a/fs/proc/array.c
 +++ b/fs/proc/array.c
 @@ -212,7 +212,7 @@ static inline void task_state(struct seq_file *m, struct 
 pid_namespace *ns,
   group_info = cred-group_info;
   task_unlock(p);
  
 - for (g = 0; g  min(group_info-ngroups, NGROUPS_SMALL); g++)
 + for (g = 0; g  group_info-ngroups; g++)
   seq_printf(m, %d ,
  from_kgid_munged(user_ns, GROUP_AT(group_info, g)));
   put_cred(cred);
 -- 
 1.7.7.6
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH] proc: pid/status: show all supplementary groups

2012-11-09 Thread Kees Cook
On Fri, Nov 9, 2012 at 5:31 AM, Artem Bityutskiy dedeki...@gmail.com wrote:
 From: Artem Bityutskiy artem.bityuts...@linux.intel.com

 We display a list of supplementary group for each process in the
 /proc/pid/status. However, we show only the first 32 groups, not all of 
 them.

 Although this is rare, but sometimes processes do have more than 32
 supplementary groups, and this kernel limitation breaks user-space apps
 that rely on the group list in /proc/pid/status.

 Number 32 comes from the internal NGROUPS_SMALL macro which defines the
 length for the internal kernel small groups buffer. There is no apparent
 reason to limit to this value.

 This patch removes the 32 groups printing limit.

 The Linux kernel limits the amount of supplementary groups by NGROUPS_MAX,
 which is currently set to 65536. And this is the maximum count of groups we
 may possibly print.

 Signed-off-by: Artem Bityutskiy artem.bityuts...@linux.intel.com

Acked-by: Kees Cook keesc...@chromium.org

 Cc: sta...@vger.kernel.org
 ---
  fs/proc/array.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)

 NOTE: I consider this to be a bug which breaks user-space, so I add -stable.

I'm not sure if this will fly since it's been broken for a very long
time, but it's a tiny change.

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/