[PATCH] sparc64: mem mmap

[PATCH] sparc64: mem mmap

[Bob Picco]

From: bob picco <bpicco@meloft.net>

This patch attempts to restrict mmap of /dev/mem. It tightens up the bus
error (DTLB errors) issues related to mmap of /dev/mem.

A root privileged application like python using dmidecode mmap-s /dev/mem to
examine an x86 magic area which is non-existent on sparc64. This results
in a power off event within sun4v_dtlb_error_report because the HV reports
an invalid RA.

The mmap restriction of /dev/mem does introduce ARCH_HAS_VALID_PHYS_ADDR_RANGE.
This does cause us to clone the functionality of valid_phys_addr_range. A
mapper is not allowed to mmap a range which isn't kern_addr_valid true on
every 4Mb boundary.

Cc: sparclinux@vger.kernel.org
Reviewed-by: Dave Kleikamp <dave.kleikamp@oracle.com>
Signed-off-by: Bob Picco <bob.picco@oracle.com>
---
 arch/sparc/include/asm/io_64.h |    3 +++
 arch/sparc/mm/init_64.c        |   27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/arch/sparc/include/asm/io_64.h b/arch/sparc/include/asm/io_64.h
index 80b54b3..3588a63 100644
--- a/arch/sparc/include/asm/io_64.h
+++ b/arch/sparc/include/asm/io_64.h
@@ -440,6 +440,9 @@ void sbus_set_sbus64(struct device *, int);
  * access
  */
 #define xlate_dev_mem_ptr(p)	__va(p)
+#define ARCH_HAS_VALID_PHYS_ADDR_RANGE
+extern int valid_phys_addr_range(phys_addr_t addr, size_t size);
+extern int valid_mmap_phys_addr_range(unsigned long pfn, size_t size);
 
 /*
  * Convert a virtual cached pointer to an uncached pointer
diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index 98ac8e8..da1f051 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c
@@ -169,6 +169,33 @@ unsigned long sparc64_valid_addr_bitmap[VALID_ADDR_BITMAP_BYTES /
 					sizeof(unsigned long)];
 EXPORT_SYMBOL(sparc64_valid_addr_bitmap);
 
+/* This is a perfect clone of mem.c's version.*/
+int valid_phys_addr_range(phys_addr_t addr, size_t count)
+{
+	return addr + count <= __pa(high_memory);
+}
+
+/* For mmap of /dev/mem we don't permit ranges with a hole which
+ * can result in a bus error. Should a mapping be required then
+ * multiple mappings with the bus error holes avoided is necessary.
+ */
+int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)
+{
+	unsigned long pa_start = pfn << PAGE_SHIFT;
+	unsigned long pa_end = pa_start + size;
+	int rc = 0;
+
+	for (pa_start = (pa_start >> ILOG2_4MB) << ILOG2_4MB;
+		pa_start < pa_end; pa_start = pa_start + (1UL << ILOG2_4MB)) {
+		unsigned long vaddr = (unsigned long) __va(pa_start);
+
+		rc = kern_addr_valid(vaddr);
+		if (!rc)
+			break;
+	}
+	return rc;
+}
+
 /* Kernel physical address base and size in bytes.  */
 unsigned long kern_base __read_mostly;
 unsigned long kern_size __read_mostly;
-- 
1.7.1

Re: [PATCH] sparc64: mem mmap

[David Miller]

From: Bob Picco <bpicco@meloft.net>
Date: Sun,  7 Sep 2014 11:48:45 -0400

> From: bob picco <bpicco@meloft.net>
> 
> This patch attempts to restrict mmap of /dev/mem. It tightens up the bus
> error (DTLB errors) issues related to mmap of /dev/mem.
> 
> A root privileged application like python using dmidecode mmap-s /dev/mem to
> examine an x86 magic area which is non-existent on sparc64. This results
> in a power off event within sun4v_dtlb_error_report because the HV reports
> an invalid RA.
> 
> The mmap restriction of /dev/mem does introduce ARCH_HAS_VALID_PHYS_ADDR_RANGE.
> This does cause us to clone the functionality of valid_phys_addr_range. A
> mapper is not allowed to mmap a range which isn't kern_addr_valid true on
> every 4Mb boundary.
> 
> Cc: sparclinux@vger.kernel.org
> Reviewed-by: Dave Kleikamp <dave.kleikamp@oracle.com>
> Signed-off-by: Bob Picco <bob.picco@oracle.com>

I think the fact that you are defining the restriction for read/write
differently for mmap should give you some pause.

There really should be no difference.

The reason you're doing this is because if we access the physical
address on the kernel side and it's a bad RA, we fault cleanly,
whereas if the user does this we do not.

And I think, given this, the real fix is to make user side accesses
recover cleanly just as the kernel ones do.

Therefore, this kind of continues the feedback for the TLB error
handling patch we've been discussing.

It seems to me that if the hypervisor errors out with "BAD RA", and we
are accessing a user page, we can just bus error the process.

What do you think?

Re: [PATCH] sparc64: mem mmap

[Bob Picco]

David Miller wrote:	[Tue Sep 09 2014, 06:07:42PM EDT]
> From: Bob Picco <bpicco@meloft.net>
> Date: Sun,  7 Sep 2014 11:48:45 -0400
> 
> > From: bob picco <bpicco@meloft.net>
> > 
> > This patch attempts to restrict mmap of /dev/mem. It tightens up the bus
> > error (DTLB errors) issues related to mmap of /dev/mem.
> > 
> > A root privileged application like python using dmidecode mmap-s /dev/mem to
> > examine an x86 magic area which is non-existent on sparc64. This results
> > in a power off event within sun4v_dtlb_error_report because the HV reports
> > an invalid RA.
> > 
> > The mmap restriction of /dev/mem does introduce ARCH_HAS_VALID_PHYS_ADDR_RANGE.
> > This does cause us to clone the functionality of valid_phys_addr_range. A
> > mapper is not allowed to mmap a range which isn't kern_addr_valid true on
> > every 4Mb boundary.
> > 
> > Cc: sparclinux@vger.kernel.org
> > Reviewed-by: Dave Kleikamp <dave.kleikamp@oracle.com>
> > Signed-off-by: Bob Picco <bob.picco@oracle.com>
> 
> I think the fact that you are defining the restriction for read/write
> differently for mmap should give you some pause.
> 
> There really should be no difference.
> 
> The reason you're doing this is because if we access the physical
> address on the kernel side and it's a bad RA, we fault cleanly,
> whereas if the user does this we do not.
> 
> And I think, given this, the real fix is to make user side accesses
> recover cleanly just as the kernel ones do.
> 
> Therefore, this kind of continues the feedback for the TLB error
> handling patch we've been discussing.
> 
> It seems to me that if the hypervisor errors out with "BAD RA", and we
> are accessing a user page, we can just bus error the process.
> 
> What do you think?
Let's abandon this patch.

In a common part for DTLB/ITLB "BAD RA" check TSTATE_PRIV. Should we not
be privileged raise a bus error with SIGBUS and BUS_ADRERR for the task.

How does this sound?

thanx,

bob
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] sparc64: mem mmap

[David Miller]

From: Bob Picco <bpicco@meloft.net>
Date: Wed, 10 Sep 2014 10:30:10 -0400

> In a common part for DTLB/ITLB "BAD RA" check TSTATE_PRIV. Should we not
> be privileged raise a bus error with SIGBUS and BUS_ADRERR for the task.

The kernel could be accessing userspace via copy_from_user() or one of
the other asm/uaccess.h accessors.

So you'll need the full context of the fault to make this decision, just
like do_sparc64_fault() does.

Actually, kernel TLB misses go via separate path and validate the RA
already before trying to load the entry into the TLB.  So you will
contextually know if we are dealing with a user mapping or not.

You could add a new thread fault code (FAULT_CODE_BAD_RA) to set in
this situation and then vector to do_sparc64_fault() when this
happens.

Probably do_sparc64_fault() should just immediately do a
do_fault_siginfo().  Suggested logic is:

	if (!down_read_trylock(&mm->mmap_sem)) {
 ...
		down_read(&mm->mmap_sem);
	}

	if (fault_code & FAULT_CODE_BAD_RA)
		goto do_sigbus;

	vma = find_vma(mm, address);

I would not try to do anything clever like seeing if this is a
non-faulting load or anything like that.

Re: [PATCH] sparc64: mem mmap

[Bob Picco]

David Miller wrote:	[Wed Sep 10 2014, 02:50:34PM EDT]
> From: Bob Picco <bpicco@meloft.net>
> Date: Wed, 10 Sep 2014 10:30:10 -0400
> 
> > In a common part for DTLB/ITLB "BAD RA" check TSTATE_PRIV. Should we not
> > be privileged raise a bus error with SIGBUS and BUS_ADRERR for the task.
> 
> The kernel could be accessing userspace via copy_from_user() or one of
> the other asm/uaccess.h accessors.
True and thanx for catching my brain slippage!
> 
> So you'll need the full context of the fault to make this decision, just
> like do_sparc64_fault() does.
Yes.
> 
> Actually, kernel TLB misses go via separate path and validate the RA
> already before trying to load the entry into the TLB.  So you will
> contextually know if we are dealing with a user mapping or not.
> 
> You could add a new thread fault code (FAULT_CODE_BAD_RA) to set in
> this situation and then vector to do_sparc64_fault() when this
> happens.
> 
> Probably do_sparc64_fault() should just immediately do a
> do_fault_siginfo().  Suggested logic is:
> 
> 	if (!down_read_trylock(&mm->mmap_sem)) {
>  ...
> 		down_read(&mm->mmap_sem);
> 	}
> 
> 	if (fault_code & FAULT_CODE_BAD_RA)
> 		goto do_sigbus;
> 
> 	vma = find_vma(mm, address);
> 
> I would not try to do anything clever like seeing if this is a
> non-faulting load or anything like that.
Okay let me look at this.

thanx again,

bob
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html