Re: [RFC][PATCH] mm: Tighten x86 /dev/mem with zeroing

2017-04-10 Thread Kees Cook 
On Thu, Apr 6, 2017 at 7:25 AM, Tommi Rantala  wrote:
> On 06.04.2017 03:00, Kees Cook wrote:
>>
>> This changes the x86 exception for the low 1MB by reading back zeros for
>> RAM areas instead of blindly allowing them. (It may be possible for heap
>> to end up getting allocated in low 1MB RAM, and then read out, possibly
>> tripping hardened usercopy.)
>>
>> Unfinished: this still needs mmap support.
>>
>> Reported-by: Tommi Rantala 
>> Signed-off-by: Kees Cook 
>> ---
>> Tommi, can you check and see if this fixes what you're seeing? I want to
>> make sure this actually works first. (x86info uses seek/read not mmap.)
>
>
> Hi, I can confirm that it works (after adding CONFIG_STRICT_DEVMEM), no more
> kernel bugs when running x86info.

Great, thanks for testing!

Linus, given that this fixes the problem, are you okay with this patch
as at least the first step? It doesn't solve the mmap exposure case,
but I'm struggling to figure out how to construct zero-page holes in
the mmap vma, and strictly speaking hardened usercopy doesn't trip
over the mmap since it's not using copy_to_user...

-Kees

>
>
> open("/dev/mem", O_RDONLY)  = 3
> lseek(3, 1038, SEEK_SET)= 1038
> read(3, "\300\235", 2)  = 2
> lseek(3, 646144, SEEK_SET)  = 646144
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024)
> = 1024
> lseek(3, 1043, SEEK_SET)= 1043
> read(3, "w\2", 2)   = 2
> lseek(3, 645120, SEEK_SET)  = 645120
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024)
> = 1024
> lseek(3, 654336, SEEK_SET)  = 654336
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024)
> = 1024
> lseek(3, 983040, SEEK_SET)  = 983040
> read(3,
> "IFE$\245S\0\0\1\0\0\0\0\360y\0\0\360\220\260\30\237{=\23\10\17\\276\17\0"...,
> 65536) = 65536
> lseek(3, 917504, SEEK_SET)  = 917504
> read(3,
> "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377"...,
> 65536) = 65536
> lseek(3, 524288, SEEK_SET)  = 524288
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
> 65536) = 65536
> lseek(3, 589824, SEEK_SET)  = 589824
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
> 65536) = 65536
>
>
> dd works too:
>
> # LANG=C dd if=/dev/mem of=/dev/null bs=4096 count=256
> 256+0 records in
> 256+0 records out
> 1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0874073 s, 12.0 MB/s
>
>
>
>> ---
>>
>>  arch/x86/mm/init.c | 41 +++
>>  drivers/char/mem.c | 82
>> ++
>>  2 files changed, 82 insertions(+), 41 deletions(-)



-- 
Kees Cook
Pixel Security

Re: [RFC][PATCH] mm: Tighten x86 /dev/mem with zeroing

2017-04-10 Thread Kees Cook 
On Thu, Apr 6, 2017 at 7:25 AM, Tommi Rantala  wrote:
> On 06.04.2017 03:00, Kees Cook wrote:
>>
>> This changes the x86 exception for the low 1MB by reading back zeros for
>> RAM areas instead of blindly allowing them. (It may be possible for heap
>> to end up getting allocated in low 1MB RAM, and then read out, possibly
>> tripping hardened usercopy.)
>>
>> Unfinished: this still needs mmap support.
>>
>> Reported-by: Tommi Rantala 
>> Signed-off-by: Kees Cook 
>> ---
>> Tommi, can you check and see if this fixes what you're seeing? I want to
>> make sure this actually works first. (x86info uses seek/read not mmap.)
>
>
> Hi, I can confirm that it works (after adding CONFIG_STRICT_DEVMEM), no more
> kernel bugs when running x86info.

Great, thanks for testing!

Linus, given that this fixes the problem, are you okay with this patch
as at least the first step? It doesn't solve the mmap exposure case,
but I'm struggling to figure out how to construct zero-page holes in
the mmap vma, and strictly speaking hardened usercopy doesn't trip
over the mmap since it's not using copy_to_user...

-Kees

>
>
> open("/dev/mem", O_RDONLY)  = 3
> lseek(3, 1038, SEEK_SET)= 1038
> read(3, "\300\235", 2)  = 2
> lseek(3, 646144, SEEK_SET)  = 646144
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024)
> = 1024
> lseek(3, 1043, SEEK_SET)= 1043
> read(3, "w\2", 2)   = 2
> lseek(3, 645120, SEEK_SET)  = 645120
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024)
> = 1024
> lseek(3, 654336, SEEK_SET)  = 654336
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 1024)
> = 1024
> lseek(3, 983040, SEEK_SET)  = 983040
> read(3,
> "IFE$\245S\0\0\1\0\0\0\0\360y\0\0\360\220\260\30\237{=\23\10\17\\276\17\0"...,
> 65536) = 65536
> lseek(3, 917504, SEEK_SET)  = 917504
> read(3,
> "\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377"...,
> 65536) = 65536
> lseek(3, 524288, SEEK_SET)  = 524288
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
> 65536) = 65536
> lseek(3, 589824, SEEK_SET)  = 589824
> read(3,
> "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"...,
> 65536) = 65536
>
>
> dd works too:
>
> # LANG=C dd if=/dev/mem of=/dev/null bs=4096 count=256
> 256+0 records in
> 256+0 records out
> 1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0874073 s, 12.0 MB/s
>
>
>
>> ---
>>
>>  arch/x86/mm/init.c | 41 +++
>>  drivers/char/mem.c | 82
>> ++
>>  2 files changed, 82 insertions(+), 41 deletions(-)



-- 
Kees Cook
Pixel Security

Re: [RFC][PATCH] mm: Tighten x86 /dev/mem with zeroing

2017-04-06 Thread Tommi Rantala 

On 06.04.2017 03:00, Kees Cook wrote:

This changes the x86 exception for the low 1MB by reading back zeros for
RAM areas instead of blindly allowing them. (It may be possible for heap
to end up getting allocated in low 1MB RAM, and then read out, possibly
tripping hardened usercopy.)

Unfinished: this still needs mmap support.

Reported-by: Tommi Rantala 
Signed-off-by: Kees Cook 
---
Tommi, can you check and see if this fixes what you're seeing? I want to
make sure this actually works first. (x86info uses seek/read not mmap.)


Hi, I can confirm that it works (after adding CONFIG_STRICT_DEVMEM), no 
more kernel bugs when running x86info.



open("/dev/mem", O_RDONLY)  = 3
lseek(3, 1038, SEEK_SET)= 1038
read(3, "\300\235", 2)  = 2
lseek(3, 646144, SEEK_SET)  = 646144
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
1024) = 1024

lseek(3, 1043, SEEK_SET)= 1043
read(3, "w\2", 2)   = 2
lseek(3, 645120, SEEK_SET)  = 645120
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
1024) = 1024

lseek(3, 654336, SEEK_SET)  = 654336
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
1024) = 1024

lseek(3, 983040, SEEK_SET)  = 983040
read(3, 
"IFE$\245S\0\0\1\0\0\0\0\360y\0\0\360\220\260\30\237{=\23\10\17\\276\17\0"..., 
65536) = 65536

lseek(3, 917504, SEEK_SET)  = 917504
read(3, 
"\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377"..., 
65536) = 65536

lseek(3, 524288, SEEK_SET)  = 524288
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
65536) = 65536

lseek(3, 589824, SEEK_SET)  = 589824
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
65536) = 65536



dd works too:

# LANG=C dd if=/dev/mem of=/dev/null bs=4096 count=256
256+0 records in
256+0 records out
1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0874073 s, 12.0 MB/s



---

 arch/x86/mm/init.c | 41 +++
 drivers/char/mem.c | 82 ++
 2 files changed, 82 insertions(+), 41 deletions(-)

Re: [RFC][PATCH] mm: Tighten x86 /dev/mem with zeroing

2017-04-06 Thread Tommi Rantala 

On 06.04.2017 03:00, Kees Cook wrote:

This changes the x86 exception for the low 1MB by reading back zeros for
RAM areas instead of blindly allowing them. (It may be possible for heap
to end up getting allocated in low 1MB RAM, and then read out, possibly
tripping hardened usercopy.)

Unfinished: this still needs mmap support.

Reported-by: Tommi Rantala 
Signed-off-by: Kees Cook 
---
Tommi, can you check and see if this fixes what you're seeing? I want to
make sure this actually works first. (x86info uses seek/read not mmap.)


Hi, I can confirm that it works (after adding CONFIG_STRICT_DEVMEM), no 
more kernel bugs when running x86info.



open("/dev/mem", O_RDONLY)  = 3
lseek(3, 1038, SEEK_SET)= 1038
read(3, "\300\235", 2)  = 2
lseek(3, 646144, SEEK_SET)  = 646144
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
1024) = 1024

lseek(3, 1043, SEEK_SET)= 1043
read(3, "w\2", 2)   = 2
lseek(3, 645120, SEEK_SET)  = 645120
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
1024) = 1024

lseek(3, 654336, SEEK_SET)  = 654336
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
1024) = 1024

lseek(3, 983040, SEEK_SET)  = 983040
read(3, 
"IFE$\245S\0\0\1\0\0\0\0\360y\0\0\360\220\260\30\237{=\23\10\17\\276\17\0"..., 
65536) = 65536

lseek(3, 917504, SEEK_SET)  = 917504
read(3, 
"\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377\377"..., 
65536) = 65536

lseek(3, 524288, SEEK_SET)  = 524288
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
65536) = 65536

lseek(3, 589824, SEEK_SET)  = 589824
read(3, 
"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 
65536) = 65536



dd works too:

# LANG=C dd if=/dev/mem of=/dev/null bs=4096 count=256
256+0 records in
256+0 records out
1048576 bytes (1.0 MB, 1.0 MiB) copied, 0.0874073 s, 12.0 MB/s



---

 arch/x86/mm/init.c | 41 +++
 drivers/char/mem.c | 82 ++
 2 files changed, 82 insertions(+), 41 deletions(-)

[RFC][PATCH] mm: Tighten x86 /dev/mem with zeroing

2017-04-05 Thread Kees Cook 
This changes the x86 exception for the low 1MB by reading back zeros for
RAM areas instead of blindly allowing them. (It may be possible for heap
to end up getting allocated in low 1MB RAM, and then read out, possibly
tripping hardened usercopy.)

Unfinished: this still needs mmap support.

Reported-by: Tommi Rantala 
Signed-off-by: Kees Cook 
---
Tommi, can you check and see if this fixes what you're seeing? I want to
make sure this actually works first. (x86info uses seek/read not mmap.)
---

 arch/x86/mm/init.c | 41 +++
 drivers/char/mem.c | 82 ++
 2 files changed, 82 insertions(+), 41 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 22af912d66d2..889e7619a091 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -643,21 +643,40 @@ void __init init_mem_mapping(void)
  * devmem_is_allowed() checks to see if /dev/mem access to a certain address
  * is valid. The argument is a physical page number.
  *
- *
- * On x86, access has to be given to the first megabyte of ram because that 
area
- * contains BIOS code and data regions used by X and dosemu and similar apps.
- * Access has to be given to non-kernel-ram areas as well, these contain the 
PCI
- * mmio resources as well as potential bios/acpi data regions.
+ * On x86, access has to be given to the first megabyte of RAM because that
+ * area traditionally contains BIOS code and data regions used by X, dosemu,
+ * and similar apps. Since they map the entire memory range, the whole range
+ * must be allowed (for mapping), but any areas that would otherwise be
+ * disallowed are flagged as being "zero filled" instead of rejected.
+ * Access has to be given to non-kernel-ram areas as well, these contain the
+ * PCI mmio resources as well as potential bios/acpi data regions.
  */
 int devmem_is_allowed(unsigned long pagenr)
 {
-   if (pagenr < 256)
-   return 1;
-   if (iomem_is_exclusive(pagenr << PAGE_SHIFT))
+   if (page_is_ram(pagenr)) {
+   /*
+* For disallowed memory regions in the low 1MB range,
+* request that the page be shown as all zeros.
+*/
+   if (pagenr < 256)
+   return 2;
+
+   return 0;
+   }
+
+   /*
+* This must follow RAM test, since System RAM is considered a
+* restricted resource under CONFIG_STRICT_IOMEM.
+*/
+   if (iomem_is_exclusive(pagenr << PAGE_SHIFT)) {
+   /* Low 1MB bypasses iomem restrictions. */
+   if (pagenr < 256)
+   return 1;
+
return 0;
-   if (!page_is_ram(pagenr))
-   return 1;
-   return 0;
+   }
+
+   return 1;
 }
 
 void free_init_pages(char *what, unsigned long begin, unsigned long end)
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 6d9cc2d39d22..7e4a9d1296bb 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -60,6 +60,10 @@ static inline int valid_mmap_phys_addr_range(unsigned long 
pfn, size_t size)
 #endif
 
 #ifdef CONFIG_STRICT_DEVMEM
+static inline int page_is_allowed(unsigned long pfn)
+{
+   return devmem_is_allowed(pfn);
+}
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
u64 from = ((u64)pfn) << PAGE_SHIFT;
@@ -75,6 +79,10 @@ static inline int range_is_allowed(unsigned long pfn, 
unsigned long size)
return 1;
 }
 #else
+static inline int page_is_allowed(unsigned long pfn)
+{
+   return 1;
+}
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
return 1;
@@ -122,23 +130,31 @@ static ssize_t read_mem(struct file *file, char __user 
*buf,
 
while (count > 0) {
unsigned long remaining;
+   int allowed;
 
sz = size_inside_page(p, count);
 
-   if (!range_is_allowed(p >> PAGE_SHIFT, count))
+   allowed = page_is_allowed(p >> PAGE_SHIFT);
+   if (!allowed)
return -EPERM;
+   if (allowed == 2) {
+   /* Show zeros for restricted memory. */
+   remaining = clear_user(buf, sz);
+   } else {
+   /*
+* On ia64 if a page has been mapped somewhere as
+* uncached, then it must also be accessed uncached
+* by the kernel or data corruption may occur.
+*/
+   ptr = xlate_dev_mem_ptr(p);
+   if (!ptr)
+   return -EFAULT;
 
-   /*
-* On ia64 if a page has been mapped somewhere as uncached, then
-* it must also be accessed uncached by the kernel or data
-* corruption may occur.
-*/
-

[RFC][PATCH] mm: Tighten x86 /dev/mem with zeroing

2017-04-05 Thread Kees Cook 
This changes the x86 exception for the low 1MB by reading back zeros for
RAM areas instead of blindly allowing them. (It may be possible for heap
to end up getting allocated in low 1MB RAM, and then read out, possibly
tripping hardened usercopy.)

Unfinished: this still needs mmap support.

Reported-by: Tommi Rantala 
Signed-off-by: Kees Cook 
---
Tommi, can you check and see if this fixes what you're seeing? I want to
make sure this actually works first. (x86info uses seek/read not mmap.)
---

 arch/x86/mm/init.c | 41 +++
 drivers/char/mem.c | 82 ++
 2 files changed, 82 insertions(+), 41 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 22af912d66d2..889e7619a091 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -643,21 +643,40 @@ void __init init_mem_mapping(void)
  * devmem_is_allowed() checks to see if /dev/mem access to a certain address
  * is valid. The argument is a physical page number.
  *
- *
- * On x86, access has to be given to the first megabyte of ram because that 
area
- * contains BIOS code and data regions used by X and dosemu and similar apps.
- * Access has to be given to non-kernel-ram areas as well, these contain the 
PCI
- * mmio resources as well as potential bios/acpi data regions.
+ * On x86, access has to be given to the first megabyte of RAM because that
+ * area traditionally contains BIOS code and data regions used by X, dosemu,
+ * and similar apps. Since they map the entire memory range, the whole range
+ * must be allowed (for mapping), but any areas that would otherwise be
+ * disallowed are flagged as being "zero filled" instead of rejected.
+ * Access has to be given to non-kernel-ram areas as well, these contain the
+ * PCI mmio resources as well as potential bios/acpi data regions.
  */
 int devmem_is_allowed(unsigned long pagenr)
 {
-   if (pagenr < 256)
-   return 1;
-   if (iomem_is_exclusive(pagenr << PAGE_SHIFT))
+   if (page_is_ram(pagenr)) {
+   /*
+* For disallowed memory regions in the low 1MB range,
+* request that the page be shown as all zeros.
+*/
+   if (pagenr < 256)
+   return 2;
+
+   return 0;
+   }
+
+   /*
+* This must follow RAM test, since System RAM is considered a
+* restricted resource under CONFIG_STRICT_IOMEM.
+*/
+   if (iomem_is_exclusive(pagenr << PAGE_SHIFT)) {
+   /* Low 1MB bypasses iomem restrictions. */
+   if (pagenr < 256)
+   return 1;
+
return 0;
-   if (!page_is_ram(pagenr))
-   return 1;
-   return 0;
+   }
+
+   return 1;
 }
 
 void free_init_pages(char *what, unsigned long begin, unsigned long end)
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 6d9cc2d39d22..7e4a9d1296bb 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -60,6 +60,10 @@ static inline int valid_mmap_phys_addr_range(unsigned long 
pfn, size_t size)
 #endif
 
 #ifdef CONFIG_STRICT_DEVMEM
+static inline int page_is_allowed(unsigned long pfn)
+{
+   return devmem_is_allowed(pfn);
+}
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
u64 from = ((u64)pfn) << PAGE_SHIFT;
@@ -75,6 +79,10 @@ static inline int range_is_allowed(unsigned long pfn, 
unsigned long size)
return 1;
 }
 #else
+static inline int page_is_allowed(unsigned long pfn)
+{
+   return 1;
+}
 static inline int range_is_allowed(unsigned long pfn, unsigned long size)
 {
return 1;
@@ -122,23 +130,31 @@ static ssize_t read_mem(struct file *file, char __user 
*buf,
 
while (count > 0) {
unsigned long remaining;
+   int allowed;
 
sz = size_inside_page(p, count);
 
-   if (!range_is_allowed(p >> PAGE_SHIFT, count))
+   allowed = page_is_allowed(p >> PAGE_SHIFT);
+   if (!allowed)
return -EPERM;
+   if (allowed == 2) {
+   /* Show zeros for restricted memory. */
+   remaining = clear_user(buf, sz);
+   } else {
+   /*
+* On ia64 if a page has been mapped somewhere as
+* uncached, then it must also be accessed uncached
+* by the kernel or data corruption may occur.
+*/
+   ptr = xlate_dev_mem_ptr(p);
+   if (!ptr)
+   return -EFAULT;
 
-   /*
-* On ia64 if a page has been mapped somewhere as uncached, then
-* it must also be accessed uncached by the kernel or data
-* corruption may occur.
-*/
-   ptr = xlate_dev_mem_ptr(p);
-