[PATCH] mm: use %pK for /proc/vmallocinfo

[Kees Cook]

In the paranoid case of sysctl kernel.kptr_restrict=2, mask the kernel
virtual addresses in /proc/vmallocinfo too.

Reported-by: Brad Spengler <spe...@grsecurity.net>
Signed-off-by: Kees Cook <kees...@chromium.org>
---
mm/vmalloc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 2bb90b1..9c871db 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -2572,7 +2572,7 @@ static int s_show(struct seq_file *m, void *p)
{
struct vm_struct *v = p;

- seq_printf(m, "0x%p-0x%p %7ld",
+ seq_printf(m, "0x%pK-0x%pK %7ld",
v->addr, v->addr + v->size, v->size);

if (v->caller)
--
1.7.9.5


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

[KOSAKI Motohiro]

On Tue, Oct 2, 2012 at 7:49 PM, Kees Cook <kees...@chromium.org> wrote:
> In the paranoid case of sysctl kernel.kptr_restrict=2, mask the kernel
> virtual addresses in /proc/vmallocinfo too.
>
> Reported-by: Brad Spengler <spe...@grsecurity.net>
> Signed-off-by: Kees Cook <kees...@chromium.org>
> ---
> mm/vmalloc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 2bb90b1..9c871db 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -2572,7 +2572,7 @@ static int s_show(struct seq_file *m, void *p)
> {
> struct vm_struct *v = p;
>
> - seq_printf(m, "0x%p-0x%p %7ld",
> + seq_printf(m, "0x%pK-0x%pK %7ld",
> v->addr, v->addr + v->size, v->size);

Looks good.
Acked-by: KOSAKI Motohiro <kosaki....@jp.fujitsu.com>

[David Rientjes]

On Tue, 2 Oct 2012, Kees Cook wrote:

> In the paranoid case of sysctl kernel.kptr_restrict=2, mask the kernel
> virtual addresses in /proc/vmallocinfo too.
>
> Reported-by: Brad Spengler <spe...@grsecurity.net>
> Signed-off-by: Kees Cook <kees...@chromium.org>

/proc/vmallocinfo is S_IRUSR, not S_IRUGO, so exactly what are you trying
to protect?

[Kees Cook]

On Tue, Oct 2, 2012 at 10:12 PM, David Rientjes <rien...@google.com> wrote:
> On Tue, 2 Oct 2012, Kees Cook wrote:
>
>> In the paranoid case of sysctl kernel.kptr_restrict=2, mask the kernel
>> virtual addresses in /proc/vmallocinfo too.
>>
>> Reported-by: Brad Spengler <spe...@grsecurity.net>
>> Signed-off-by: Kees Cook <kees...@chromium.org>
>
> /proc/vmallocinfo is S_IRUSR, not S_IRUGO, so exactly what are you trying
> to protect?

Trying to block the root user from seeing virtual memory addresses
(mode 2 of kptr_restrict).

Documentation/sysctl/kernel.txt:
"This toggle indicates whether restrictions are placed on
exposing kernel addresses via /proc and other interfaces. When
kptr_restrict is set to (0), there are no restrictions. When
kptr_restrict is set to (1), the default, kernel pointers
printed using the %pK format specifier will be replaced with 0's
unless the user has CAP_SYSLOG. When kptr_restrict is set to
(2), kernel pointers printed using %pK will be replaced with 0's
regardless of privileges."

Even though it's S_IRUSR, it still needs %pK for the paranoid case.

-Kees

--
Kees Cook
Chrome OS Security

[David Rientjes]

On Tue, 2 Oct 2012, Kees Cook wrote:

> >> In the paranoid case of sysctl kernel.kptr_restrict=2, mask the kernel
> >> virtual addresses in /proc/vmallocinfo too.
> >>
> >> Reported-by: Brad Spengler <spe...@grsecurity.net>
> >> Signed-off-by: Kees Cook <kees...@chromium.org>
> >
> > /proc/vmallocinfo is S_IRUSR, not S_IRUGO, so exactly what are you trying
> > to protect?
>
> Trying to block the root user from seeing virtual memory addresses
> (mode 2 of kptr_restrict).
>
> Documentation/sysctl/kernel.txt:
> "This toggle indicates whether restrictions are placed on
> exposing kernel addresses via /proc and other interfaces. When
> kptr_restrict is set to (0), there are no restrictions. When
> kptr_restrict is set to (1), the default, kernel pointers
> printed using the %pK format specifier will be replaced with 0's
> unless the user has CAP_SYSLOG. When kptr_restrict is set to
> (2), kernel pointers printed using %pK will be replaced with 0's
> regardless of privileges."
>
> Even though it's S_IRUSR, it still needs %pK for the paranoid case.
>

So root does echo 0 > /proc/sys/kernel/kptr_restrict first. Again: what

are you trying to protect?

[Kees Cook]

On Tue, Oct 2, 2012 at 10:37 PM, David Rientjes <rien...@google.com> wrote:
> On Tue, 2 Oct 2012, Kees Cook wrote:
>
>> >> In the paranoid case of sysctl kernel.kptr_restrict=2, mask the kernel
>> >> virtual addresses in /proc/vmallocinfo too.
>> >>
>> >> Reported-by: Brad Spengler <spe...@grsecurity.net>
>> >> Signed-off-by: Kees Cook <kees...@chromium.org>
>> >
>> > /proc/vmallocinfo is S_IRUSR, not S_IRUGO, so exactly what are you trying
>> > to protect?
>>
>> Trying to block the root user from seeing virtual memory addresses
>> (mode 2 of kptr_restrict).
>>
>> Documentation/sysctl/kernel.txt:
>> "This toggle indicates whether restrictions are placed on
>> exposing kernel addresses via /proc and other interfaces. When
>> kptr_restrict is set to (0), there are no restrictions. When
>> kptr_restrict is set to (1), the default, kernel pointers
>> printed using the %pK format specifier will be replaced with 0's
>> unless the user has CAP_SYSLOG. When kptr_restrict is set to
>> (2), kernel pointers printed using %pK will be replaced with 0's
>> regardless of privileges."
>>
>> Even though it's S_IRUSR, it still needs %pK for the paranoid case.
>
> So root does echo 0 > /proc/sys/kernel/kptr_restrict first. Again: what
> are you trying to protect?

Only CAP_SYS_ADMIN can change the setting. This is, for example, for
containers, or other situations where a uid 0 process lacking
CAP_SYS_ADMIN cannot see virtual addresses. It's a very paranoid case,
yes, but it's part of how this feature was designed. Think of it as
supporting the recent uid 0 vs ring 0 boundary. :)

-Kees

--
Kees Cook
Chrome OS Security

[David Rientjes]

On Wed, 3 Oct 2012, Kees Cook wrote:

> > So root does echo 0 > /proc/sys/kernel/kptr_restrict first. Again: what
> > are you trying to protect?
>
> Only CAP_SYS_ADMIN can change the setting. This is, for example, for
> containers, or other situations where a uid 0 process lacking
> CAP_SYS_ADMIN cannot see virtual addresses. It's a very paranoid case,
> yes, but it's part of how this feature was designed. Think of it as
> supporting the recent uid 0 vs ring 0 boundary. :)
>

The intention of /proc/vmallocinfo being S_IRUSR is obviously to only
allow root to read this information to begin with, so if root lacks
CAP_SYS_ADMIN then it seems the best fix would be to return an empty file
on read()? Or give permission to everybody to read it but only return a
positive count when they have CAP_SYS_ADMIN?

There's no need to make this so convoluted that you need to have the right
combination of uid, kptr_restrict, CAP_SYS_ADMIN, and CAP_SYSLOG to get
anything valuable out of this file, though.

[Kees Cook]

On Wed, Oct 3, 2012 at 11:02 AM, David Rientjes <rien...@google.com> wrote:
> On Wed, 3 Oct 2012, Kees Cook wrote:
>
>> > So root does echo 0 > /proc/sys/kernel/kptr_restrict first. Again: what
>> > are you trying to protect?
>>
>> Only CAP_SYS_ADMIN can change the setting. This is, for example, for
>> containers, or other situations where a uid 0 process lacking
>> CAP_SYS_ADMIN cannot see virtual addresses. It's a very paranoid case,
>> yes, but it's part of how this feature was designed. Think of it as
>> supporting the recent uid 0 vs ring 0 boundary. :)
>>
>
> The intention of /proc/vmallocinfo being S_IRUSR is obviously to only
> allow root to read this information to begin with, so if root lacks
> CAP_SYS_ADMIN then it seems the best fix would be to return an empty file
> on read()? Or give permission to everybody to read it but only return a
> positive count when they have CAP_SYS_ADMIN?
>
> There's no need to make this so convoluted that you need to have the right
> combination of uid, kptr_restrict, CAP_SYS_ADMIN, and CAP_SYSLOG to get
> anything valuable out of this file, though.

Well, the existing mechanism is using %pK. I see no reason to add
additional complexity.

-Kees

--
Kees Cook
Chrome OS Security