[PATCH] powerpc/powernv: Validate memcons descriptor and output buffer

[PATCH] powerpc/powernv: Validate memcons descriptor and output buffer

[Gavin Shan]

Currently, it's assumed that memcons and its output buffer are included
in the linear mapping. It's not true when "mem=384M" is included in
bootargs. The system runs into kernel crash eventually.

   # od -x /proc/device-tree/ibm,opal/ibm,opal-memcons
   0000000 0000 0000 0b30 0010
   0000010

This validates memcons descriptor and its output buffer to ensure they
are valid in linear mapping. Otherwise, the interface won't be populated
to avoid kernel crash during system boot.

Cc: stable@vger.kernel.org #3.15+
Fixes: bfc36894a48 ("powerpc/powernv: Add OPAL message log interface")
Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
---
 arch/powerpc/platforms/powernv/opal-msglog.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/powernv/opal-msglog.c b/arch/powerpc/platforms/powernv/opal-msglog.c
index 39d6ff9..34dc2f2 100644
--- a/arch/powerpc/platforms/powernv/opal-msglog.c
+++ b/arch/powerpc/platforms/powernv/opal-msglog.c
@@ -15,6 +15,7 @@
 #include <linux/of.h>
 #include <linux/types.h>
 #include <asm/barrier.h>
+#include <asm/setup.h>
 
 /* OPAL in-memory console. Defined in OPAL source at core/console.c */
 struct memcons {
@@ -104,7 +105,7 @@ static struct bin_attribute opal_msglog_attr = {
 
 void __init opal_msglog_init(void)
 {
-	u64 mcaddr;
+	u64 mcaddr, obuf_top;
 	struct memcons *mc;
 
 	if (of_property_read_u64(opal_node, "ibm,opal-memcons", &mcaddr)) {
@@ -112,6 +113,12 @@ void __init opal_msglog_init(void)
 		return;
 	}
 
+	if (memory_limit && (mcaddr + sizeof(*mc)) > memory_limit) {
+		pr_warn("OPAL: memcons descriptor (0x%llx, 0x%lx) is out of memory (0x%llx)\n",
+			mcaddr, sizeof(*mc), memory_limit);
+		return;
+	}
+
 	mc = phys_to_virt(mcaddr);
 	if (!mc) {
 		pr_warn("OPAL: memory console address is invalid\n");
@@ -123,6 +130,13 @@ void __init opal_msglog_init(void)
 		return;
 	}
 
+	obuf_top = be64_to_cpu(mc->obuf_phys) + be32_to_cpu(mc->obuf_size);
+	if (memory_limit && obuf_top > memory_limit) {
+		pr_warn("OPAL: memcons output buffer ceiling (0x%llx) is out of memory (0x%llx)\n",
+			obuf_top, memory_limit);
+		return;
+	}
+
 	opal_memcons = mc;
 }
 
-- 
2.7.4

Re: [PATCH] powerpc/powernv: Validate memcons descriptor and output buffer

[Michael Ellerman]

Gavin Shan <gwshan@linux.vnet.ibm.com> writes:

> Currently, it's assumed that memcons and its output buffer are included
> in the linear mapping. It's not true when "mem=384M" is included in
> bootargs. The system runs into kernel crash eventually.
>
>    # od -x /proc/device-tree/ibm,opal/ibm,opal-memcons
>    0000000 0000 0000 0b30 0010
>    0000010
>
> This validates memcons descriptor and its output buffer to ensure they
> are valid in linear mapping. Otherwise, the interface won't be populated
> to avoid kernel crash during system boot.
>
> Cc: stable@vger.kernel.org #3.15+
> Fixes: bfc36894a48 ("powerpc/powernv: Add OPAL message log interface")
> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>

Hmm.

Arguably the memcons shouldn't be in the linear map at all.

AFAICS the kernel only ever reads from it, so really it should be
explicitly mapped, and mapped read only.

That would also fix this problem :D

cheers

Re: [PATCH] powerpc/powernv: Validate memcons descriptor and output buffer

[Gavin Shan]

On Tue, Jan 24, 2017 at 09:21:22PM +1100, Michael Ellerman wrote:
>Gavin Shan <gwshan@linux.vnet.ibm.com> writes:
>
>> Currently, it's assumed that memcons and its output buffer are included
>> in the linear mapping. It's not true when "mem=384M" is included in
>> bootargs. The system runs into kernel crash eventually.
>>
>>    # od -x /proc/device-tree/ibm,opal/ibm,opal-memcons
>>    0000000 0000 0000 0b30 0010
>>    0000010
>>
>> This validates memcons descriptor and its output buffer to ensure they
>> are valid in linear mapping. Otherwise, the interface won't be populated
>> to avoid kernel crash during system boot.
>>
>> Cc: stable@vger.kernel.org #3.15+
>> Fixes: bfc36894a48 ("powerpc/powernv: Add OPAL message log interface")
>> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
>
>Hmm.
>
>Arguably the memcons shouldn't be in the linear map at all.
>

Thanks for review. I agree it shouldn't be in linear map as it is for logs
produced by skiboot, not kernel. Currently, the regions (including the
reserved ones) in memblock are covered by linear mapping. As I can see,
there is no flag to differentiate the cases - the region is reserved by
skiboot or kernel itself. With the flag, we can skip mapping the regions
that were reserved by skiboot to linear mapping area in htab_initialize()
or radix_init_pgtable(). I will see if I can introduce on without too much
efforts.

As I can see, the only benefit is to avoid randomly writing to the area
and data corruption, if the region is invisible from linear mapping.

>AFAICS the kernel only ever reads from it, so really it should be
>explicitly mapped, and mapped read only.
>
>That would also fix this problem :D
>

Yes, I think your proposed solution is better. This patch just adds more
checks to avoid the problem, meaning the logs is inaccessible if the buffer
is out of range. With your suggestion, it's still accessible.

I believe the log buffer should be mapped to VMALLOC area. In order to
setup mapping, we need page frame (struct page). However, it's possible
the page frame isn't existing (e.g. with mem= in bootargs).

It seems the best option would be mapping it into IOREMAP area with
ioremap_prot(). No page frame is needed and allow to set the region
to READONLY. Michael, what's your thoughts about this? :-)

Thanks,
Gavin