2012-11-09 13:30:08

by Artem Bityutskiy

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

From: Artem Bityutskiy <[email protected]>

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


2012-11-09 15:29:04

by Serge Hallyn

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

Quoting Artem Bityutskiy ([email protected]):
> From: Artem Bityutskiy <[email protected]>
>
> 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 <[email protected]>

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

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

2012-11-09 18:30:59

by Kees Cook

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

On Fri, Nov 9, 2012 at 5:31 AM, Artem Bityutskiy <[email protected]> wrote:
> From: Artem Bityutskiy <[email protected]>
>
> 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 <[email protected]>

Acked-by: Kees Cook <[email protected]>

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