Without enabling the workaround for ARM errata 430973 thumb
compiled userland crashes randomly on the Nokia N900.
Signed-off-by: Sebastian Reichel <[email protected]>
---
Hi Tony,
I think this patch should be added into some fix branch for 3.14-rcX.
-- Sebastian
---
arch/arm/mach-omap2/pdata-quirks.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
index 3d5b24d..7929df3 100644
--- a/arch/arm/mach-omap2/pdata-quirks.c
+++ b/arch/arm/mach-omap2/pdata-quirks.c
@@ -22,6 +22,8 @@
#include "common-board-devices.h"
#include "dss-common.h"
#include "control.h"
+#include "omap-secure.h"
+#include "soc.h"
struct pdata_init {
const char *compatible;
@@ -169,6 +171,21 @@ static void __init am3517_evm_legacy_init(void)
omap_ctrl_writel(v, AM35XX_CONTROL_IP_SW_RESET);
omap_ctrl_readl(AM35XX_CONTROL_IP_SW_RESET); /* OCP barrier */
}
+
+static void __init nokia_n900_legacy_init(void)
+{
+ hsmmc2_internal_input_clk();
+
+ if (omap_type() == OMAP2_DEVICE_TYPE_SEC) {
+ if (IS_ENABLED(CONFIG_ARM_ERRATA_430973)) {
+ pr_info("RX-51: Enabling ARM errata 430973 workaround\n");
+ /* set IBE to 1 */
+ rx51_secure_update_aux_cr(BIT(6), 0);
+ } else {
+ pr_warning("RX-51: Not enabling ARM errata 430973 workaround\n");
+ }
+ }
+}
#endif /* CONFIG_ARCH_OMAP3 */
#ifdef CONFIG_ARCH_OMAP4
@@ -259,7 +276,7 @@ struct of_dev_auxdata omap_auxdata_lookup[] __initdata = {
static struct pdata_init pdata_quirks[] __initdata = {
#ifdef CONFIG_ARCH_OMAP3
{ "compulab,omap3-sbc-t3730", omap3_sbc_t3730_legacy_init, },
- { "nokia,omap3-n900", hsmmc2_internal_input_clk, },
+ { "nokia,omap3-n900", nokia_n900_legacy_init, },
{ "nokia,omap3-n9", hsmmc2_internal_input_clk, },
{ "nokia,omap3-n950", hsmmc2_internal_input_clk, },
{ "isee,omap3-igep0020", omap3_igep0020_legacy_init, },
--
1.8.5.3
2014-02-05 Sebastian Reichel <[email protected]>:
> Without enabling the workaround for ARM errata 430973 thumb
> compiled userland crashes randomly on the Nokia N900.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> Hi Tony,
>
> I think this patch should be added into some fix branch for 3.14-rcX.
>
> -- Sebastian
> ---
> arch/arm/mach-omap2/pdata-quirks.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
> index 3d5b24d..7929df3 100644
> --- a/arch/arm/mach-omap2/pdata-quirks.c
> +++ b/arch/arm/mach-omap2/pdata-quirks.c
> @@ -22,6 +22,8 @@
> #include "common-board-devices.h"
> #include "dss-common.h"
> #include "control.h"
> +#include "omap-secure.h"
> +#include "soc.h"
>
> struct pdata_init {
> const char *compatible;
> @@ -169,6 +171,21 @@ static void __init am3517_evm_legacy_init(void)
> omap_ctrl_writel(v, AM35XX_CONTROL_IP_SW_RESET);
> omap_ctrl_readl(AM35XX_CONTROL_IP_SW_RESET); /* OCP barrier */
> }
> +
> +static void __init nokia_n900_legacy_init(void)
> +{
> + hsmmc2_internal_input_clk();
> +
> + if (omap_type() == OMAP2_DEVICE_TYPE_SEC) {
> + if (IS_ENABLED(CONFIG_ARM_ERRATA_430973)) {
> + pr_info("RX-51: Enabling ARM errata 430973 workaround\n");
> + /* set IBE to 1 */
> + rx51_secure_update_aux_cr(BIT(6), 0);
> + } else {
> + pr_warning("RX-51: Not enabling ARM errata 430973 workaround\n");
> + }
> + }
> +}
> #endif /* CONFIG_ARCH_OMAP3 */
>
> #ifdef CONFIG_ARCH_OMAP4
> @@ -259,7 +276,7 @@ struct of_dev_auxdata omap_auxdata_lookup[] __initdata = {
> static struct pdata_init pdata_quirks[] __initdata = {
> #ifdef CONFIG_ARCH_OMAP3
> { "compulab,omap3-sbc-t3730", omap3_sbc_t3730_legacy_init, },
> - { "nokia,omap3-n900", hsmmc2_internal_input_clk, },
> + { "nokia,omap3-n900", nokia_n900_legacy_init, },
> { "nokia,omap3-n9", hsmmc2_internal_input_clk, },
> { "nokia,omap3-n950", hsmmc2_internal_input_clk, },
> { "isee,omap3-igep0020", omap3_igep0020_legacy_init, },
> --
> 1.8.5.3
>
Hello,
I have two questions:
1) Why are you using if (IS_ENABLED(CONFIG_ARM_ERRATA_430973)) instead #ifdef ?
2) Why do you not write warning or info when omap type is not
OMAP2_DEVICE_TYPE_SEC (e.g qemu) ?
--
Pali Rohár
[email protected]
Hi Pali,
On Wed, Feb 05, 2014 at 02:12:56PM +0100, Pali Roh?r wrote:
> 1) Why are you using if (IS_ENABLED(CONFIG_ARM_ERRATA_430973)) instead #ifdef ?
This is the preferred kernel style as far as I know.
> 2) Why do you not write warning or info when omap type is not
> OMAP2_DEVICE_TYPE_SEC (e.g qemu) ?
I assumed, that the workaround is not needed for this device type.
I just added the warning for missing CONFIG_ARM_ERRATA_430973,
because its very likely a misconfigured kernel.
-- Sebastian
2014-02-05 Sebastian Reichel <[email protected]>:
> Hi Pali,
>
> On Wed, Feb 05, 2014 at 02:12:56PM +0100, Pali Rohár wrote:
>> 1) Why are you using if (IS_ENABLED(CONFIG_ARM_ERRATA_430973)) instead #ifdef ?
>
> This is the preferred kernel style as far as I know.
>
Ok.
>> 2) Why do you not write warning or info when omap type is not
>> OMAP2_DEVICE_TYPE_SEC (e.g qemu) ?
>
> I assumed, that the workaround is not needed for this device type.
>
That rx51 secure call must not be called on non secure devices (e.g.
qemu), because it cause kernel crash. So I thought that kernel should
write something like secure call is disabled on that device types.
Kernel code for errata 430973 will update ibe bit for non secure
devices.
> I just added the warning for missing CONFIG_ARM_ERRATA_430973,
> because its very likely a misconfigured kernel.
>
> -- Sebastian
Yes, it can be misconfigured kernel, but if you do not have any thumb
binary (like stock Maemo 5 system), then it is safe and OK.
--
Pali Rohár
[email protected]
Hi,
On Wed, Feb 05, 2014 at 05:38:54PM +0100, Pali Roh?r wrote:
> > I assumed, that the workaround is not needed for this device type.
>
> That rx51 secure call must not be called on non secure devices (e.g.
> qemu), because it cause kernel crash. So I thought that kernel should
> write something like secure call is disabled on that device types.
> Kernel code for errata 430973 will update ibe bit for non secure
> devices.
Do you see any advantage in having that message?
> > I just added the warning for missing CONFIG_ARM_ERRATA_430973,
> > because its very likely a misconfigured kernel.
>
> Yes, it can be misconfigured kernel, but if you do not have any thumb
> binary (like stock Maemo 5 system), then it is safe and OK.
I think running this kernel may also be a potential security
problem. If I understand it right the ARM core is left in an
unstable state when you run Thumb code, so this may result in
funny effects in the kernel?
-- Sebastian
Hi,
On 5.02.2014 19:17, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Feb 05, 2014 at 05:38:54PM +0100, Pali Roh?r wrote:
>>> I assumed, that the workaround is not needed for this device type.
>>
>> That rx51 secure call must not be called on non secure devices (e.g.
>> qemu), because it cause kernel crash. So I thought that kernel should
>> write something like secure call is disabled on that device types.
>> Kernel code for errata 430973 will update ibe bit for non secure
>> devices.
>
> Do you see any advantage in having that message?
>
AIUI it will appear only when booting the kernel in "qemu -m rx51...", I
am not aware of any other non-secure device manifesting itself as RX51.
So there is little advantage of having that additional message IMO.
>>> I just added the warning for missing CONFIG_ARM_ERRATA_430973,
>>> because its very likely a misconfigured kernel.
>>
>> Yes, it can be misconfigured kernel, but if you do not have any thumb
>> binary (like stock Maemo 5 system), then it is safe and OK.
>
> I think running this kernel may also be a potential security
> problem. If I understand it right the ARM core is left in an
> unstable state when you run Thumb code, so this may result in
> funny effects in the kernel?
>
> -- Sebastian
>
In theory having that workaround disabled might be a security problem,
but honestly, knowing its nature I don't think it is easily exploitable,
if at all. The final result when bitten by it is a SIGILL, but in
userspace, not in the kernel(assuming the kernel is ARM), and userspace
runs in totally different mode (nonsecure, nonprivileged) compared to
the kernel(nonsecure, privileged) and IIRC every mode has its own set of
stack, registers etc. BTW I don't think the kernel itself can be
thumb2-compiled for cores with that errata, but I might wrong. Also, as
Pali noted, the problem appears if and only if there is an userspace
binary containing thumb2 code. If all of the userspace is pure ARM,
there is no problem. And as the errata workaround has its drawbacks (BTB
is cleared on every context switch which affects performance), one might
want to not have it enabled. Maybe that warning should be spit only if
CONFIG_THUMB2_KERNEL (or whatever the option was) is enabled. Though if
that option is enabled I'd rather #error during compile time if errata
workaround is not enabled, instead of printing a warning while booting a
system that will crash in a matter of seconds.
Ivo
2014-02-05 Sebastian Reichel <[email protected]>:
> Hi,
>
> On Wed, Feb 05, 2014 at 05:38:54PM +0100, Pali Rohár wrote:
>> > I assumed, that the workaround is not needed for this device type.
>>
>> That rx51 secure call must not be called on non secure devices (e.g.
>> qemu), because it cause kernel crash. So I thought that kernel should
>> write something like secure call is disabled on that device types.
>> Kernel code for errata 430973 will update ibe bit for non secure
>> devices.
>
> Do you see any advantage in having that message?
>
Consistency. Print message 1) only if secure rx51 smc instruction is
called or 2) print message always (also if smc is not called). With
that patch message is printed (enabled/disabled) only if device is
rx51 and device type is HS.
For information it is also good to know that in qemu that special
secure smc instruction was not called.
--
Pali Rohár
[email protected]
On Wed, Feb 05, 2014 at 09:42:30PM +0100, Pali Roh?r wrote:
> 2014-02-05 Sebastian Reichel <[email protected]>:
> > Hi,
> >
> > On Wed, Feb 05, 2014 at 05:38:54PM +0100, Pali Roh?r wrote:
> >> > I assumed, that the workaround is not needed for this device type.
> >>
> >> That rx51 secure call must not be called on non secure devices (e.g.
> >> qemu), because it cause kernel crash. So I thought that kernel should
> >> write something like secure call is disabled on that device types.
> >> Kernel code for errata 430973 will update ibe bit for non secure
> >> devices.
> >
> > Do you see any advantage in having that message?
> >
>
> Consistency. Print message 1) only if secure rx51 smc instruction is
> called or 2) print message always (also if smc is not called). With
> that patch message is printed (enabled/disabled) only if device is
> rx51 and device type is HS.
>
> For information it is also good to know that in qemu that special
> secure smc instruction was not called.
Assuming I do not know about this errata/workaround and have some
problems with crashing binaries I may have a look at the kernel
warnings. Without the additional message I won't get any hint about
the errata/workaround at all.
On the other hand there is no such information gain on qemu. It just
tells the user, that some workaround, which is not needed, has not
been activated.
-- Sebastian
Hi,
On Wed, Feb 05, 2014 at 10:30:52PM +0200, Ivaylo Dimitrov wrote:
> In theory having that workaround disabled might be a security
> problem, but honestly, knowing its nature I don't think it is easily
> exploitable, if at all. The final result when bitten by it is a
> SIGILL, but in userspace, not in the kernel(assuming the kernel is
> ARM), and userspace runs in totally different mode (nonsecure,
> nonprivileged) compared to the kernel(nonsecure, privileged) and
> IIRC every mode has its own set of stack, registers etc. BTW I don't
> think the kernel itself can be thumb2-compiled for cores with that
> errata, but I might wrong. Also, as Pali noted, the problem appears
> if and only if there is an userspace binary containing thumb2 code.
> If all of the userspace is pure ARM, there is no problem.
Ok. I assumed, that a single thumb2-compiled binary can potentially
trigger problems for the whole system. Basically because the ERRATA
helptext in KConfig is:
[...], Cortex-A8 does not recover from the stale interworking branch
prediction.
> And as the errata workaround has its drawbacks (BTB is cleared on
> every context switch which affects performance), one might want to
> not have it enabled.
I guess the workaround would be persistently enabled if there were
no drawbacks.
> Maybe that warning should be spit only if CONFIG_THUMB2_KERNEL (or
> whatever the option was) is enabled. Though if that option is
> enabled I'd rather #error during compile time if errata workaround
> is not enabled, instead of printing a warning while booting a
> system that will crash in a matter of seconds.
THUMB2 userland code is supported by the kernel by default, so that
does not work. (Enabling CONFIG_THUMB2_KERNEL will result in a
thumb2-compiled kernel.)
-- Sebastian
On Wed 2014-02-05 13:33:02, Sebastian Reichel wrote:
> Without enabling the workaround for ARM errata 430973 thumb
> compiled userland crashes randomly on the Nokia N900.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
Reviewed-by: Pavel Machek <[email protected]>
> +static void __init nokia_n900_legacy_init(void)
> +{
> + hsmmc2_internal_input_clk();
> +
> + if (omap_type() == OMAP2_DEVICE_TYPE_SEC) {
> + if (IS_ENABLED(CONFIG_ARM_ERRATA_430973)) {
> + pr_info("RX-51: Enabling ARM errata 430973 workaround\n");
> + /* set IBE to 1 */
> + rx51_secure_update_aux_cr(BIT(6), 0);
> + } else {
> + pr_warning("RX-51: Not enabling ARM errata 430973 workaround\n");
> + }
I'd use higher loglevel there, and added "thumb binaries can now crash
randomly, and can crash the system" ... or something like that, if you
are going to send new version of the patch.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Without enabling the workaround for ARM errata 430973 thumb
compiled userland crashes randomly on the Nokia N900.
Signed-off-by: Sebastian Reichel <[email protected]>
Reviewed-by: Pavel Machek <[email protected]>
---
Hi,
This is PATCHv2 for the thumb errata workaround for N900 DT
boot, which should go into 3.14-rc.
Changes since PATCHv1:
* Add Reviewed-By: Pavel Machek
* Add more verbose warning about the errata as suggested by Pavel
I did not increase the verbose level, since the next higher verbose
level is pr_err and the message is not an error.
I think it would make sense to update the Kconfig entry for the errata to
enable the workaround by default for the Nokia N900. That should be done in
its own patch, though.
Apart from that it would be nice if errata workaround could be enabled
via DeviceTree instead of kernel configuration. This will require some
discussion though, so I think we should start with this patch for now.
-- Sebastian
---
arch/arm/mach-omap2/pdata-quirks.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
index 3d5b24d..0cc710d 100644
--- a/arch/arm/mach-omap2/pdata-quirks.c
+++ b/arch/arm/mach-omap2/pdata-quirks.c
@@ -22,6 +22,8 @@
#include "common-board-devices.h"
#include "dss-common.h"
#include "control.h"
+#include "omap-secure.h"
+#include "soc.h"
struct pdata_init {
const char *compatible;
@@ -169,6 +171,22 @@ static void __init am3517_evm_legacy_init(void)
omap_ctrl_writel(v, AM35XX_CONTROL_IP_SW_RESET);
omap_ctrl_readl(AM35XX_CONTROL_IP_SW_RESET); /* OCP barrier */
}
+
+static void __init nokia_n900_legacy_init(void)
+{
+ hsmmc2_internal_input_clk();
+
+ if (omap_type() == OMAP2_DEVICE_TYPE_SEC) {
+ if (IS_ENABLED(CONFIG_ARM_ERRATA_430973)) {
+ pr_info("RX-51: Enabling ARM errata 430973 workaround\n");
+ /* set IBE to 1 */
+ rx51_secure_update_aux_cr(BIT(6), 0);
+ } else {
+ pr_warning("RX-51: Not enabling ARM errata 430973 workaround\n");
+ pr_warning("Thumb binaries may crash randomly without this workaround\n");
+ }
+ }
+}
#endif /* CONFIG_ARCH_OMAP3 */
#ifdef CONFIG_ARCH_OMAP4
@@ -259,7 +277,7 @@ struct of_dev_auxdata omap_auxdata_lookup[] __initdata = {
static struct pdata_init pdata_quirks[] __initdata = {
#ifdef CONFIG_ARCH_OMAP3
{ "compulab,omap3-sbc-t3730", omap3_sbc_t3730_legacy_init, },
- { "nokia,omap3-n900", hsmmc2_internal_input_clk, },
+ { "nokia,omap3-n900", nokia_n900_legacy_init, },
{ "nokia,omap3-n9", hsmmc2_internal_input_clk, },
{ "nokia,omap3-n950", hsmmc2_internal_input_clk, },
{ "isee,omap3-igep0020", omap3_igep0020_legacy_init, },
--
1.8.5.3
* Sebastian Reichel <[email protected]> [140217 13:33]:
> Without enabling the workaround for ARM errata 430973 thumb
> compiled userland crashes randomly on the Nokia N900.
>
> Signed-off-by: Sebastian Reichel <[email protected]>
> Reviewed-by: Pavel Machek <[email protected]>
> ---
> Hi,
>
> This is PATCHv2 for the thumb errata workaround for N900 DT
> boot, which should go into 3.14-rc.
>
> Changes since PATCHv1:
> * Add Reviewed-By: Pavel Machek
> * Add more verbose warning about the errata as suggested by Pavel
>
> I did not increase the verbose level, since the next higher verbose
> level is pr_err and the message is not an error.
>
> I think it would make sense to update the Kconfig entry for the errata to
> enable the workaround by default for the Nokia N900. That should be done in
> its own patch, though.
>
> Apart from that it would be nice if errata workaround could be enabled
> via DeviceTree instead of kernel configuration. This will require some
> discussion though, so I think we should start with this patch for now.
Thanks applying into omap-for-v3.14/fixes.
Tony