[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Marek Vasut]

Initially reported by Detlev Zundel <dzu@denx.de>, this caused breakage
when "su - testuser" was called. "testuser" being a regular user account,
the command ended with SIGKILL.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Wolfgang Denk <wd@denx.de>
---
 arch/arm/configs/mxs_defconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
index ccdb635..f212802 100644
--- a/arch/arm/configs/mxs_defconfig
+++ b/arch/arm/configs/mxs_defconfig
@@ -34,7 +34,7 @@ CONFIG_NO_HZ=y
 CONFIG_HIGH_RES_TIMERS=y
 CONFIG_PREEMPT_VOLUNTARY=y
 CONFIG_AEABI=y
-CONFIG_DEFAULT_MMAP_MIN_ADDR=65536
+CONFIG_DEFAULT_MMAP_MIN_ADDR=32768
 CONFIG_AUTO_ZRELADDR=y
 CONFIG_FPE_NWFPE=y
 CONFIG_NET=y
-- 
1.7.10.4

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Fabio Estevam]

Marek,

On Fri, Aug 3, 2012 at 12:55 PM, Marek Vasut <marex@denx.de> wrote:
> Initially reported by Detlev Zundel <dzu@denx.de>, this caused breakage
> when "su - testuser" was called. "testuser" being a regular user account,
> the command ended with SIGKILL.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: Wolfgang Denk <wd@denx.de>
> ---
>  arch/arm/configs/mxs_defconfig |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
> index ccdb635..f212802 100644
> --- a/arch/arm/configs/mxs_defconfig
> +++ b/arch/arm/configs/mxs_defconfig
> @@ -34,7 +34,7 @@ CONFIG_NO_HZ=y
>  CONFIG_HIGH_RES_TIMERS=y
>  CONFIG_PREEMPT_VOLUNTARY=y
>  CONFIG_AEABI=y
> -CONFIG_DEFAULT_MMAP_MIN_ADDR=65536
> +CONFIG_DEFAULT_MMAP_MIN_ADDR=32768

No need to set it to 32768.

If you just remove the 'CONFIG_DEFAULT_MMAP_MIN_ADDR=65536' then this
symbol will be 32768.

So your patch would become:

diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
index ccdb635..4edcfb4 100644
--- a/arch/arm/configs/mxs_defconfig
+++ b/arch/arm/configs/mxs_defconfig
@@ -34,7 +34,6 @@ CONFIG_NO_HZ=y
 CONFIG_HIGH_RES_TIMERS=y
 CONFIG_PREEMPT_VOLUNTARY=y
 CONFIG_AEABI=y
-CONFIG_DEFAULT_MMAP_MIN_ADDR=65536
 CONFIG_AUTO_ZRELADDR=y
 CONFIG_FPE_NWFPE=y
 CONFIG_NET=y

Regards,

Fabio Estevam

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Fabio Estevam]

On Fri, Aug 3, 2012 at 1:46 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Marek,
>
> On Fri, Aug 3, 2012 at 12:55 PM, Marek Vasut <marex@denx.de> wrote:
>> Initially reported by Detlev Zundel <dzu@denx.de>, this caused breakage
>> when "su - testuser" was called. "testuser" being a regular user account,
>> the command ended with SIGKILL.

Also, please point to the commit that introduced such breakage.

Thanks,

Fabio Estevam

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Fabio Estevam]

On Fri, Aug 3, 2012 at 1:46 PM, Fabio Estevam <festevam@gmail.com> wrote:
> Marek,
>
> On Fri, Aug 3, 2012 at 12:55 PM, Marek Vasut <marex@denx.de> wrote:
>> Initially reported by Detlev Zundel <dzu@denx.de>, this caused breakage
>> when "su - testuser" was called. "testuser" being a regular user account,
>> the command ended with SIGKILL.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Shawn Guo <shawn.guo@linaro.org>
>> Cc: Wolfgang Denk <wd@denx.de>
>> ---
>>  arch/arm/configs/mxs_defconfig |    2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
>> index ccdb635..f212802 100644
>> --- a/arch/arm/configs/mxs_defconfig
>> +++ b/arch/arm/configs/mxs_defconfig
>> @@ -34,7 +34,7 @@ CONFIG_NO_HZ=y
>>  CONFIG_HIGH_RES_TIMERS=y
>>  CONFIG_PREEMPT_VOLUNTARY=y
>>  CONFIG_AEABI=y
>> -CONFIG_DEFAULT_MMAP_MIN_ADDR=65536
>> +CONFIG_DEFAULT_MMAP_MIN_ADDR=32768
>
> No need to set it to 32768.
>
> If you just remove the 'CONFIG_DEFAULT_MMAP_MIN_ADDR=65536' then this
> symbol will be 32768.

Sorry, ignore what I said. I just realized that the default is 4096.

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Russell King - ARM Linux]

On Fri, Aug 03, 2012 at 01:54:07PM -0300, Fabio Estevam wrote:
> On Fri, Aug 3, 2012 at 1:46 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > Marek,
> >
> > On Fri, Aug 3, 2012 at 12:55 PM, Marek Vasut <marex@denx.de> wrote:
> >> Initially reported by Detlev Zundel <dzu@denx.de>, this caused breakage
> >> when "su - testuser" was called. "testuser" being a regular user account,
> >> the command ended with SIGKILL.
> >>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
> >> Cc: Russell King <linux@arm.linux.org.uk>
> >> Cc: Shawn Guo <shawn.guo@linaro.org>
> >> Cc: Wolfgang Denk <wd@denx.de>
> >> ---
> >>  arch/arm/configs/mxs_defconfig |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/configs/mxs_defconfig b/arch/arm/configs/mxs_defconfig
> >> index ccdb635..f212802 100644
> >> --- a/arch/arm/configs/mxs_defconfig
> >> +++ b/arch/arm/configs/mxs_defconfig
> >> @@ -34,7 +34,7 @@ CONFIG_NO_HZ=y
> >>  CONFIG_HIGH_RES_TIMERS=y
> >>  CONFIG_PREEMPT_VOLUNTARY=y
> >>  CONFIG_AEABI=y
> >> -CONFIG_DEFAULT_MMAP_MIN_ADDR=65536
> >> +CONFIG_DEFAULT_MMAP_MIN_ADDR=32768
> >
> > No need to set it to 32768.
> >
> > If you just remove the 'CONFIG_DEFAULT_MMAP_MIN_ADDR=65536' then this
> > symbol will be 32768.
> 
> Sorry, ignore what I said. I just realized that the default is 4096.

4096 is also fine for ARM too.  There's not much point in having
defconfigs change it - that would just be pure noise in the config
files.

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Fabio Estevam]

On Fri, Aug 3, 2012 at 2:40 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> 4096 is also fine for ARM too.  There's not much point in having
> defconfigs change it - that would just be pure noise in the config
> files.

Thanks for the clarification, Russell.

Marek, so maybe you can send a v2 of this patch with my previous
suggestion (and also mentioning the offending commit).

I have also removed CONFIG_DEFAULT_MMAP_MIN_ADDR from imx_v6_v7_defconfig.

Regards,

Fabio Estevam

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Marek Vasut]

Dear Russell King - ARM Linux,

[...]

> > > No need to set it to 32768.
> > > 
> > > If you just remove the 'CONFIG_DEFAULT_MMAP_MIN_ADDR=65536' then this
> > > symbol will be 32768.
> > 
> > Sorry, ignore what I said. I just realized that the default is 4096.
> 
> 4096 is also fine for ARM too.  There's not much point in having
> defconfigs change it - that would just be pure noise in the config
> files.

Wasn't there a security concern being the reason for setting this higher? Also, 
I still don't completely understand why ARM has to set it lower than others, is 
that an ABI thing?

Russell, do you happen to have some further reading for me I could study on this 
topic please?

Thanks!

Best regards,
Marek Vasut

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Russell King - ARM Linux]

On Fri, Aug 03, 2012 at 08:46:27PM +0200, Marek Vasut wrote:
> Dear Russell King - ARM Linux,
> 
> [...]
> 
> > > > No need to set it to 32768.
> > > > 
> > > > If you just remove the 'CONFIG_DEFAULT_MMAP_MIN_ADDR=65536' then this
> > > > symbol will be 32768.
> > > 
> > > Sorry, ignore what I said. I just realized that the default is 4096.
> > 
> > 4096 is also fine for ARM too.  There's not much point in having
> > defconfigs change it - that would just be pure noise in the config
> > files.
> 
> Wasn't there a security concern being the reason for setting this higher?

I don't believe there is.  The only requirement is that the first page
on older CPUs isn't stomped on (and we preserve that for later CPUs so
that NULL pointer derefs get caught.)

The higher it is the better though, because it means NULL pointer + offset
deref with larger offsets also gets caught.

> Also, 
> I still don't completely understand why ARM has to set it lower than
> others, is that an ABI thing?

Yes, we have always loaded user programs at 0x8000 by default, which are
generally not relocatable.  So if you set it to 64K, you prevent non-root
user programs being mapped in, which is why stuff gets killed as soon as
UID != 0.

[PATCH] MXS: Fix mxs_defconfig MMAP_MIN_ADDR

[Marek Vasut]

Dear Russell King - ARM Linux,

> On Fri, Aug 03, 2012 at 08:46:27PM +0200, Marek Vasut wrote:
> > Dear Russell King - ARM Linux,
> > 
> > [...]
> > 
> > > > > No need to set it to 32768.
> > > > > 
> > > > > If you just remove the 'CONFIG_DEFAULT_MMAP_MIN_ADDR=65536' then
> > > > > this symbol will be 32768.
> > > > 
> > > > Sorry, ignore what I said. I just realized that the default is 4096.
> > > 
> > > 4096 is also fine for ARM too.  There's not much point in having
> > > defconfigs change it - that would just be pure noise in the config
> > > files.
> > 
> > Wasn't there a security concern being the reason for setting this higher?
> 
> I don't believe there is.  The only requirement is that the first page
> on older CPUs isn't stomped on (and we preserve that for later CPUs so
> that NULL pointer derefs get caught.)
> 
> The higher it is the better though, because it means NULL pointer + offset
> deref with larger offsets also gets caught.

Understood!

> > Also,
> > I still don't completely understand why ARM has to set it lower than
> > others, is that an ABI thing?
> 
> Yes, we have always loaded user programs at 0x8000 by default, which are
> generally not relocatable.  So if you set it to 64K, you prevent non-root
> user programs being mapped in, which is why stuff gets killed as soon as
> UID != 0.

I see! Thanks for explaining!

Best regards,
Marek Vasut