In the paranoid case of sysctl kernel.kptr_restrict=2, mask the kernel
virtual addresses in /proc/vmallocinfo too.
Reported-by: Brad Spengler <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
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
On Tue, Oct 2, 2012 at 7:49 PM, Kees Cook <[email protected]> wrote:
> In the paranoid case of sysctl kernel.kptr_restrict=2, mask the kernel
> virtual addresses in /proc/vmallocinfo too.
>
> Reported-by: Brad Spengler <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
/proc/vmallocinfo is S_IRUSR, not S_IRUGO, so exactly what are you trying
to protect?
On Tue, Oct 2, 2012 at 10:12 PM, David Rientjes <[email protected]> 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 <[email protected]>
>> Signed-off-by: Kees Cook <[email protected]>
>
> /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
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 <[email protected]>
> >> Signed-off-by: Kees Cook <[email protected]>
> >
> > /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?
On Tue, Oct 2, 2012 at 10:37 PM, David Rientjes <[email protected]> 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 <[email protected]>
>> >> Signed-off-by: Kees Cook <[email protected]>
>> >
>> > /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
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.
On Wed, Oct 3, 2012 at 11:02 AM, David Rientjes <[email protected]> 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