Skip to contentSkip to conversation

Punch a hole in /dev/mem for librtas

PATCH In review Sukadev Bhattiprolu 3 participants 4 messages Dec 2, 2011 – Dec 5, 2011 PowerPC

lore
Share & tools
lore

Sukadev Bhattiprolu

author
#0 lore raw

PATCH 1/1 Punch a hole in /dev/mem for librtas

Subject: [PATCH 1/1] Punch a hole in /dev/mem for librtas

With CONFIG_STRICT_DEVMEM=y, user space cannot read any part of /dev/mem. Since this breaks librtas, punch a hole in /dev/mem to allow access to the rmo_buffer that librtas needs.

Anton Blanchard reported the problem and helped with the fix.

A quick test for this patch:

       # cat /proc/rtas/rmo_buffer
       000000000f190000 10000
       # python -c "print 0x000000000f190000 / 0x10000"
       3865
       # dd if=/dev/mem of=/tmp/foo count=1 bs=64k skip=3865
       1+0 records in
       1+0 records out
       65536 bytes (66 kB) copied, 0.000205235 s, 319 MB/s
       # dd if=/dev/mem of=/tmp/foo
       dd: reading `/dev/mem': Operation not permitted
       0+0 records in
       0+0 records out
       0 bytes (0 B) copied, 0.00022519 s, 0.0 kB/s

Changelog[v3]: [Ben Harrenschmidt]: Incremental patch for the punched hole,
since CONFIG_STRICT_DEVMEM was merged into the -next branch.

[Ben Harrenschmidt]: Rename interface to page_is_rtas_user_buf() move declaration to a header file and ensure it doesn't break CONFIG_PPC_RTAS=n.

Signed-off-by Sukadev Bhattiprolu <[email protected]>
---
 arch/powerpc/include/asm/rtas.h |   12 ++++++++++++
 arch/powerpc/mm/mem.c           |    3 +++
 2 files changed, 15 insertions(+), 0 deletions(-)
arch/powerpc/include/asm/rtas.h +12 -0
index 58625d1..c2d3c9f 100644
@@ -305,5 +305,17 @@ static inline u32 rtas_config_addr(int busno, int devfn, int reg)
 extern void __cpuinit rtas_give_timebase(void);
 extern void __cpuinit rtas_take_timebase(void);
 
+#ifdef CONFIG_PPC_RTAS
+static inline int page_is_rtas_user_buf(unsigned long pfn)
+{
+	unsigned long paddr = (pfn << PAGE_SHIFT);
+	if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf + RTAS_RMOBUF_MAX))
+		return 1;
+	return 0;
+}
+#else
+static inline int page_is_rtas_user_buf(unsigned long pfn) { return 0;}
+#endif
+
 #endif /* __KERNEL__ */
 #endif /* _POWERPC_RTAS_H */
arch/powerpc/mm/mem.c +3 -0
index 553fb41..05abd49 100644
@@ -50,6 +50,7 @@
 #include <asm/vdso.h>
 #include <asm/fixmap.h>
 #include <asm/swiotlb.h>
+#include <asm/rtas.h>
 
 #include "mmu_decl.h"
 

@@ -564,6 +565,8 @@ int devmem_is_allowed(unsigned long pfn)
 		return 0;
 	if (!page_is_ram(pfn))
 		return 1;
+	if (page_is_rtas_user_buf(pfn))
+		return 1;
 	return 0;
 }
 #endif /* CONFIG_STRICT_DEVMEM */
Signature · 1 line
1.7.0.4
#1 lore raw
Quoted 5 lines
arch/powerpc/include/asm/rtas.h +4 -0
+static inline int page_is_rtas_user_buf(unsigned long pfn)
+{
+	unsigned long paddr = (pfn << PAGE_SHIFT);
+	if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf +

RTAS_RMOBUF_MAX))

It probably cannot overflow with actual values of rtas_rmo_buf and RTAS_RMOBUF_MAX, but otherwise it is an incorrect test; please write

	if (paddr >= rtas_rmo_buf && paddr - rtas_rmo_buf < RTAS_RMOBUF_MAX)

(and, _MAX? Shouldn't it be the actual size here? Or is _MAX just a confusing name :-) )

Segher

#2 lore raw
Quoted 10 lines · Segher Boessenkool
Quoted 5 lines
arch/powerpc/include/asm/rtas.h +4 -0
+static inline int page_is_rtas_user_buf(unsigned long pfn)
+{
+	unsigned long paddr = (pfn << PAGE_SHIFT);
+	if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf +

RTAS_RMOBUF_MAX))

It probably cannot overflow with actual values of rtas_rmo_buf and RTAS_RMOBUF_MAX, but otherwise it is an incorrect test; please write

Quoted 4 lines
	if (paddr >= rtas_rmo_buf && paddr - rtas_rmo_buf < RTAS_RMOBUF_MAX)

(and, _MAX? Shouldn't it be the actual size here? Or is _MAX just a confusing name :-) )

The original code is a lot more readable and perfectly correct for all possible values of rtas_rmo_buf :-)

Cheers,
Ben.

#3 lore raw
Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
arch/powerpc/include/asm/rtas.h +4 -0
+static inline int page_is_rtas_user_buf(unsigned long pfn)
+{
+	unsigned long paddr = (pfn << PAGE_SHIFT);
+	if (paddr >= rtas_rmo_buf && paddr < (rtas_rmo_buf +

RTAS_RMOBUF_MAX))

It probably cannot overflow with actual values of rtas_rmo_buf and RTAS_RMOBUF_MAX, but otherwise it is an incorrect test; please write

Quoted 7 lines
Quoted 4 lines
	if (paddr >= rtas_rmo_buf && paddr - rtas_rmo_buf < RTAS_RMOBUF_MAX)

(and, _MAX? Shouldn't it be the actual size here? Or is _MAX just a confusing name :-) )

The original code is a lot more readable and perfectly correct for all possible values of rtas_rmo_buf :-)

You have to consider those possible values before you see it cannot overflow. So no, it's a lot _less_ readable IMHO.

It's also a dangerous habit to get into. Just say no :-)

Segher