Skip to contentSkip to conversation

ARM: fix string functions on !MMU

PATCH New Rabin Vincent 3 participants 10 messages Apr 21, 2014 – Jun 3, 2014 Arm (32-bit)

lore
Share & tools
lore

Rabin Vincent

author
#0 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Signed-off-by Rabin Vincent <[email protected]>
---
 arch/arm/include/asm/uaccess.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
arch/arm/include/asm/uaccess.h +4 -3
index 12c3a5d..c3a65f1 100644
@@ -199,6 +199,9 @@ extern int __put_user_8(void *, unsigned long long);
 		__put_user_check(x,p);					\
 	 })
 
+#define user_addr_max() \
+	(segment_eq(get_fs(), USER_DS) ? TASK_SIZE : ~0UL)
+
 #else /* CONFIG_MMU */
 
 /*

@@ -210,6 +213,7 @@ extern int __put_user_8(void *, unsigned long long);
 #define __addr_ok(addr)		((void)(addr),1)
 #define __range_ok(addr,size)	((void)(addr),0)
 #define get_fs()		(KERNEL_DS)
+#define user_addr_max()		(~0UL)
 
 static inline void set_fs(mm_segment_t fs)
 {

@@ -222,9 +226,6 @@ static inline void set_fs(mm_segment_t fs)
 
 #define access_ok(type,addr,size)	(__range_ok(addr,size) == 0)
 
-#define user_addr_max() \
-	(segment_eq(get_fs(), USER_DS) ? TASK_SIZE : ~0UL)
-
 /*
  * The "__xxx" versions of the user access functions do not verify the
  * address space - it must have been done previously with a separate
Signature · 1 line
1.9.1
#1 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

Quoted 7 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 (since it's a 32-bit quantity), so checks like addr < user_addr_max() will fail. Anyway, the code looks fine to me:

Acked-by Will Deacon <[email protected]>

Will

author
#2 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

Quoted 6 lines · Will Deacon
Quoted 5 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

Quoted 6 lines
Quoted 2 lines

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 (since it's a 32-bit quantity), so checks like addr < user_addr_max() will fail.

Thanks for the ack, but I don't quite understand what you mean here. You describe the state before this patch, right? Why does it matter that KERNEL_DS is 0x0?

#3 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

Hi Rabin,

Quoted 10 lines · Rabin Vincent
Quoted 9 lines · Will Deacon
Quoted 7 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Quoted 7 lines
Quoted 3 lines

Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 (since it's a 32-bit quantity), so checks like addr < user_addr_max() will fail.

Thanks for the ack, but I don't quite understand what you mean here. You describe the state before this patch, right? Why does it matter that KERNEL_DS is 0x0?

Apologies, I misread the code that you're patching so I guess I'll have to revoke my ack, sorry! That's not to say I think the patch is bad, I'd just like to discuss it with you a bit more.

Having re-read the code, the issue is because TASK_SIZE is defined as CONFIG_DRAM_SIZE, right? Since CONFIG_DRAM_BASE may be non-zero, it means TASK_SIZE is truly a size -- not a limit (as it would be in virtual space). What we actually want for !MMU is END_MEM instead of TASK_SIZE.

Will

author
#4 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

Quoted 19 lines · Will Deacon
Quoted 10 lines · Rabin Vincent
Quoted 9 lines · Will Deacon
Quoted 7 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Quoted 7 lines
Quoted 3 lines

Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 (since it's a 32-bit quantity), so checks like addr < user_addr_max() will fail.

Thanks for the ack, but I don't quite understand what you mean here. You describe the state before this patch, right? Why does it matter that KERNEL_DS is 0x0?

Quoted 8 lines

Apologies, I misread the code that you're patching so I guess I'll have to revoke my ack, sorry! That's not to say I think the patch is bad, I'd just like to discuss it with you a bit more.

Having re-read the code, the issue is because TASK_SIZE is defined as CONFIG_DRAM_SIZE, right? Since CONFIG_DRAM_BASE may be non-zero, it means TASK_SIZE is truly a size -- not a limit (as it would be in virtual space). What we actually want for !MMU is END_MEM instead of TASK_SIZE.

I guess you mean I should #define user_addr_max() to END_MEM for !MMU? That won't work. For example, on a !MMU boot, the first thing that fails because of this bug is devtmpsfs_mount() -> sys_mount() -> copy_mount_string() -> strndup_user(), which is run in a kernel thread. The type argument of sys_mount() points to a string in flash since we run an XIP kernel. This flash address has no relation to END_MEM (which is nothing but CONFIG_DRAM_BASE + CONFIG_DRAM_SIZE) and could be higher than END_MEM.

#5 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

Quoted 29 lines · Rabin Vincent
Quoted 19 lines · Will Deacon
Quoted 10 lines · Rabin Vincent
Quoted 9 lines · Will Deacon
Quoted 7 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Quoted 7 lines
Quoted 3 lines

Might be worth mentioning that this is an issue because KERNEL_DS is 0x0 (since it's a 32-bit quantity), so checks like addr < user_addr_max() will fail.

Thanks for the ack, but I don't quite understand what you mean here. You describe the state before this patch, right? Why does it matter that KERNEL_DS is 0x0?

Quoted 8 lines

Apologies, I misread the code that you're patching so I guess I'll have to revoke my ack, sorry! That's not to say I think the patch is bad, I'd just like to discuss it with you a bit more.

Having re-read the code, the issue is because TASK_SIZE is defined as CONFIG_DRAM_SIZE, right? Since CONFIG_DRAM_BASE may be non-zero, it means TASK_SIZE is truly a size -- not a limit (as it would be in virtual space). What we actually want for !MMU is END_MEM instead of TASK_SIZE.

Quoted 8 lines

I guess you mean I should #define user_addr_max() to END_MEM for !MMU? That won't work. For example, on a !MMU boot, the first thing that fails because of this bug is devtmpsfs_mount() -> sys_mount() -> copy_mount_string() -> strndup_user(), which is run in a kernel thread. The type argument of sys_mount() points to a string in flash since we run an XIP kernel. This flash address has no relation to END_MEM (which is nothing but CONFIG_DRAM_BASE + CONFIG_DRAM_SIZE) and could be higher than END_MEM.

Good point, I hadn't considered XIP with flash regions. In which case, I don't think we have any choice but to use ~0UL as the limit. It looks like Uwe has taken a quick look at some other users of TASK_SIZE, so I'll leave you two to complete the audit ;)

Will

#6 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

In reply to Rabin Vincent

Hello Rabin,

Quoted 9 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Signed-off-by Rabin Vincent <[email protected]>

I tested this on my efm32 machine and it booted just fine. Before I used a patch that did:

arch/arm/include/asm/memory.h +4 -1
index 02fa2558f662..f25c7f4c5a44 100644
@@ -92,9 +92,12 @@
  * It is difficult to define and perhaps will never meet the original meaning
  * of this define that was meant to.
  * Fortunately, there is no reference for this in noMMU mode, for now.
+ *
+ * HACK: copy_from_user must even handle copying from flash. So don't impose a
+ * limit at all. Not sure this is correct ...
  */
 #ifndef TASK_SIZE
-#define TASK_SIZE              (CONFIG_DRAM_SIZE)
+#define TASK_SIZE              (~0UL)
 #endif
 
 #ifndef TASK_UNMAPPED_BASE

Regarding "TASK_SIZE has anyway no meaning on !MMU", there are a few more usages of TASK_SIZE for no-MMU (tested by removing its definition and compiling with my efm32 config, so I might have missed some usages):

- mm/nommu.c uses TASK_SIZE in validate_mmap_request:

        /* Careful about overflows.. */
        rlen = PAGE_ALIGN(len);
        if (!rlen || rlen > TASK_SIZE)
                return -ENOMEM;

Maybe this should better be explicitly:

        if (!rlen || rlen > CONFIG_DRAM_SIZE)
                return -ENOMEM;

?
- kernel/sys.c uses TASK_SIZE in prctl_set_mm
used for prctl syscall with option=PR_SET_MM. Maybe here it would be nice to have TASK_SIZE == ~0UL?

- fs/exec.c uses TASK_SIZE in setup_new_exec to assign current->mm->task_size. I didn't check if/how this is used.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
author
#7 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

Quoted 19 lines · Uwe Kleine-K?nig
Quoted 9 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Signed-off-by Rabin Vincent <[email protected]>

I tested this on my efm32 machine and it booted just fine. Before I used a patch that did:

arch/arm/include/asm/memory.h +0 -0
index 02fa2558f662..f25c7f4c5a44 100644
@@ -92,9 +92,12 @@
  * It is difficult to define and perhaps will never meet the original meaning

arch/arm/include/asm/memory.h line 101

Quoted 10 lines
arch/arm/include/asm/memory.h +4 -1
  * of this define that was meant to.
  * Fortunately, there is no reference for this in noMMU mode, for now.
+ *
+ * HACK: copy_from_user must even handle copying from flash. So don't impose a
+ * limit at all. Not sure this is correct ...
  */
 #ifndef TASK_SIZE
-#define TASK_SIZE              (CONFIG_DRAM_SIZE)
+#define TASK_SIZE              (~0UL)
 #endif

The current code for user_addr_max() for !MMU is essentialy:

	#define user_addr_max() TASK_SIZE

which is obviously wrong for the KERNEL_DS case, since it should be ~0UL. And user space can access all that the kernel does, so there should be no restriction for USER_DS either (which is anyway equivalent to KERNEL_DS). Hence, I think my patch, which removes the usage of TASK_SIZE in user_addr_max() for !MMU, is correct regardless of what the correct definition or meaning of TASK_SIZE for !MMU is.

If you make TASK_SIZE to ~0UL (which is probably what it should be on !MMU), then the result is equivalent to my patch but it is not semantically correct since you are restricting user_addr_max() to TASK_SIZE even for the KERNEL_DS.

What do you say?

#8 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

Hello Rabin,

Quoted 42 lines · Rabin Vincent
Quoted 19 lines · Uwe Kleine-K?nig
Quoted 9 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Signed-off-by Rabin Vincent <[email protected]>

I tested this on my efm32 machine and it booted just fine. Before I used a patch that did:

arch/arm/include/asm/memory.h +0 -0
index 02fa2558f662..f25c7f4c5a44 100644
@@ -92,9 +92,12 @@
  * It is difficult to define and perhaps will never meet the original meaning

arch/arm/include/asm/memory.h line 101

Quoted 10 lines
arch/arm/include/asm/memory.h +4 -1
  * of this define that was meant to.
  * Fortunately, there is no reference for this in noMMU mode, for now.
+ *
+ * HACK: copy_from_user must even handle copying from flash. So don't impose a
+ * limit at all. Not sure this is correct ...
  */
 #ifndef TASK_SIZE
-#define TASK_SIZE              (CONFIG_DRAM_SIZE)
+#define TASK_SIZE              (~0UL)
 #endif

The current code for user_addr_max() for !MMU is essentialy:

	#define user_addr_max() TASK_SIZE

which is obviously wrong for the KERNEL_DS case, since it should be ~0UL. And user space can access all that the kernel does, so there should be no restriction for USER_DS either (which is anyway equivalent to KERNEL_DS). Hence, I think my patch, which removes the usage of TASK_SIZE in user_addr_max() for !MMU, is correct regardless of what the correct definition or meaning of TASK_SIZE for !MMU is.

Quoted 4 lines

If you make TASK_SIZE to ~0UL (which is probably what it should be on !MMU), then the result is equivalent to my patch but it is not semantically correct since you are restricting user_addr_max() to TASK_SIZE even for the KERNEL_DS.

I'd prefer to share as much code as possible between MMU and !MMU, so my preferred solution is:

	#ifndef CONFIG_MMU
	#define TASK_SIZE	~0UL	/* do we need parentesis? */
	#endif

	#define user_addr_max()	\
		(segment_eq(get_fs(), KERNEL_DS) ? ~0UL : TASK_SIZE)

which should be correct and address your concern.

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#9 lore raw

PATCH 1/1 ARM: fix string functions on !MMU

Hello,

Quoted 50 lines · Uwe Kleine-K?nig
Quoted 42 lines · Rabin Vincent
Quoted 19 lines · Uwe Kleine-K?nig
Quoted 9 lines · Rabin Vincent

8c56cc8be5b38e ("ARM: 7449/1: use generic strnlen_user and strncpy_from_user functions") apparently broken those string operations for !MMU. USER_DS == KERNEL_DS on !MMU, so user_addr_max() always restricts the addresses to TASK_SIZE.

TASK_SIZE has anyway no meaning on !MMU, so make user_addr_max() not restrict anything.

Signed-off-by Rabin Vincent <[email protected]>

I tested this on my efm32 machine and it booted just fine. Before I used a patch that did:

arch/arm/include/asm/memory.h +0 -0
index 02fa2558f662..f25c7f4c5a44 100644
@@ -92,9 +92,12 @@
  * It is difficult to define and perhaps will never meet the original meaning

arch/arm/include/asm/memory.h line 101

Quoted 10 lines
arch/arm/include/asm/memory.h +4 -1
  * of this define that was meant to.
  * Fortunately, there is no reference for this in noMMU mode, for now.
+ *
+ * HACK: copy_from_user must even handle copying from flash. So don't impose a
+ * limit at all. Not sure this is correct ...
  */
 #ifndef TASK_SIZE
-#define TASK_SIZE              (CONFIG_DRAM_SIZE)
+#define TASK_SIZE              (~0UL)
 #endif

The current code for user_addr_max() for !MMU is essentialy:

	#define user_addr_max() TASK_SIZE

which is obviously wrong for the KERNEL_DS case, since it should be ~0UL. And user space can access all that the kernel does, so there should be no restriction for USER_DS either (which is anyway equivalent to KERNEL_DS). Hence, I think my patch, which removes the usage of TASK_SIZE in user_addr_max() for !MMU, is correct regardless of what the correct definition or meaning of TASK_SIZE for !MMU is.

Quoted 4 lines

If you make TASK_SIZE to ~0UL (which is probably what it should be on !MMU), then the result is equivalent to my patch but it is not semantically correct since you are restricting user_addr_max() to TASK_SIZE even for the KERNEL_DS.

I'd prefer to share as much code as possible between MMU and !MMU, so my preferred solution is:

Quoted 6 lines
	#ifndef CONFIG_MMU
	#define TASK_SIZE	~0UL	/* do we need parentesis? */
	#endif

	#define user_addr_max()	\
		(segment_eq(get_fs(), KERNEL_DS) ? ~0UL : TASK_SIZE)

After looking into that a bit more I wonder if the correct version is (maybe an equivalent to):

	#define user_addr_max() \
		(segment_eq(get_fs(), KERNEL_DS) ? ~0UL : get_fs())

That is because in the MMU case get_fs() is #defined as:

	#define get_fs()        (current_thread_info()->addr_limit)

and .addr_limit is changeable via set_fs. This would also mean that the current definition:

	#define user_addr_max() \
		(segment_eq(get_fs(), USER_DS) ? TASK_SIZE : ~0UL)

might return ~0UL even though there is a limit which just happens not to be TASK_SIZE. (BTW, alpha, m68k, openrisc and sparc use the same definition.)

Thoughts?

Best regards
Uwe

Signature · 2 lines
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |