diff mbox

ARM: fix string functions on !MMU

Message ID 20140428075149.GB28564@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Uwe Kleine-König April 28, 2014, 7:51 a.m. UTC 
Hello Rabin,

On Mon, Apr 21, 2014 at 08:10:08PM +0200, Rabin Vincent wrote:
> 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 <rabin@rab.in>
I tested this on my efm32 machine and it booted just fine. Before I used
a patch that did:


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

Comments

Rabin Vincent June 2, 2014, 4:53 p.m. UTC | #1 
On Mon, Apr 28, 2014 at 09:51:49AM +0200, Uwe Kleine-König wrote:
> On Mon, Apr 21, 2014 at 08:10:08PM +0200, Rabin Vincent wrote:
> > 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 <rabin@rab.in>
> I tested this on my efm32 machine and it booted just fine. Before I used
> a patch that did:
> 
> diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> index 02fa2558f662..f25c7f4c5a44 100644
> --- a/arch/arm/include/asm/memory.h
> +++ b/arch/arm/include/asm/memory.h
> @@ -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

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?
Uwe Kleine-König June 3, 2014, 7:51 a.m. UTC | #2 
Hello Rabin,

On Mon, Jun 02, 2014 at 06:53:43PM +0200, Rabin Vincent wrote:
> On Mon, Apr 28, 2014 at 09:51:49AM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 21, 2014 at 08:10:08PM +0200, Rabin Vincent wrote:
> > > 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 <rabin@rab.in>
> > I tested this on my efm32 machine and it booted just fine. Before I used
> > a patch that did:
> > 
> > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > index 02fa2558f662..f25c7f4c5a44 100644
> > --- a/arch/arm/include/asm/memory.h
> > +++ b/arch/arm/include/asm/memory.h
> > @@ -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
> 
> 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.
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
Uwe Kleine-König June 3, 2014, 7:47 p.m. UTC | #3 
Hello,

On Tue, Jun 03, 2014 at 09:51:33AM +0200, Uwe Kleine-König wrote:
> On Mon, Jun 02, 2014 at 06:53:43PM +0200, Rabin Vincent wrote:
> > On Mon, Apr 28, 2014 at 09:51:49AM +0200, Uwe Kleine-König wrote:
> > > On Mon, Apr 21, 2014 at 08:10:08PM +0200, Rabin Vincent wrote:
> > > > 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 <rabin@rab.in>
> > > I tested this on my efm32 machine and it booted just fine. Before I used
> > > a patch that did:
> > > 
> > > diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
> > > index 02fa2558f662..f25c7f4c5a44 100644
> > > --- a/arch/arm/include/asm/memory.h
> > > +++ b/arch/arm/include/asm/memory.h
> > > @@ -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
> > 
> > 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.
> 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)

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
diff mbox

Patch

diff --git a/arch/arm/include/asm/memory.h b/arch/arm/include/asm/memory.h
index 02fa2558f662..f25c7f4c5a44 100644
--- a/arch/arm/include/asm/memory.h
+++ b/arch/arm/include/asm/memory.h
@@ -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