[PATCH v4 04/12] ARM: entry: Fix iWMMXT TIF flag handling

[PATCH v4 04/12] ARM: entry: Fix iWMMXT TIF flag handling

[Ard Biesheuvel]

The conditional MOVS instruction that appears to have been added to test
for the TIF_USING_IWMMXT thread_info flag only sets the N and Z
condition flags and register R7, none of which are referenced in the
subsequent code. This means that the instruction does nothing, which
means that we might misidentify faulting FPE instructions as iWMMXT
instructions on kernels that were built to support both.

This seems to have been part of the original submission of the code, and
so this has never worked as intended, and nobody ever noticed, and so we
might decide to just leave this as-is. However, with the ongoing move
towards multiplatform kernels, the issue becomes more likely to
manifest, and so it is better to fix it.

So check whether we are dealing with an undef exception regarding
coprocessor index #0 or #1, and if so, load the thread_info flag and
only dispatch it as a iWMMXT trap if the flag is set.

Cc: <stable@vger.kernel.org> # v2.6.9+
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm/kernel/entry-armv.S | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index c39303e5c23470e6..c5d2f07994fb0d87 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -606,10 +606,11 @@ call_fpe:
 	strb	r7, [r6, #TI_USED_CP]		@ set appropriate used_cp[]
 #ifdef CONFIG_IWMMXT
 	@ Test if we need to give access to iWMMXt coprocessors
-	ldr	r5, [r10, #TI_FLAGS]
-	rsbs	r7, r8, #(1 << 8)		@ CP 0 or 1 only
-	movscs	r7, r5, lsr #(TIF_USING_IWMMXT + 1)
-	bcs	iwmmxt_task_enable
+	tst	r8, #0xe << 8			@ CP 0 or 1?
+	ldreq	r5, [r10, #TI_FLAGS]		@ if so, load thread_info flags
+	andeq	r5, r5, #1 << TIF_USING_IWMMXT	@ isolate TIF_USING_IWMMXT flag
+	teqeq	r5, #1 << TIF_USING_IWMMXT	@ check whether it is set
+	beq	iwmmxt_task_enable		@ branch if set
 #endif
  ARM(	add	pc, pc, r8, lsr #6	)
  THUMB(	lsr	r8, r8, #6		)
-- 
2.39.2

Re: [PATCH v4 04/12] ARM: entry: Fix iWMMXT TIF flag handling

[Linus Walleij]

On Mon, Mar 20, 2023 at 2:19 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> The conditional MOVS instruction that appears to have been added to test
> for the TIF_USING_IWMMXT thread_info flag only sets the N and Z
> condition flags and register R7, none of which are referenced in the
> subsequent code. This means that the instruction does nothing, which
> means that we might misidentify faulting FPE instructions as iWMMXT
> instructions on kernels that were built to support both.
>
> This seems to have been part of the original submission of the code, and
> so this has never worked as intended, and nobody ever noticed, and so we
> might decide to just leave this as-is. However, with the ongoing move
> towards multiplatform kernels, the issue becomes more likely to
> manifest, and so it is better to fix it.
>
> So check whether we are dealing with an undef exception regarding
> coprocessor index #0 or #1, and if so, load the thread_info flag and
> only dispatch it as a iWMMXT trap if the flag is set.
>
> Cc: <stable@vger.kernel.org> # v2.6.9+
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Looks right to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

The code dates back before git history, but it was introduced in
Linux v2.6.9 and it looks like Nicolas Pitre wrote it so let's just
ping him and see what he remembers about this!

Yours,
Linus Walleij

Re: [PATCH v4 04/12] ARM: entry: Fix iWMMXT TIF flag handling

[Nicolas Pitre]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

On Mon, Mar 20, 2023 at 2:19 PM Ard Biesheuvel <ardb@kernel.org> wrote:

> The conditional MOVS instruction that appears to have been added to test
> for the TIF_USING_IWMMXT thread_info flag only sets the N and Z
> condition flags and register R7, none of which are referenced in the
> subsequent code.

Really?

As far as I know, the rsb instruction is a "reversed subtract" and that 
also sets the carry flag.

And so does a move with a shifter argument (the last dropped bit is 
moved to the carry flag).

What am I missing?


Nicolas

Re: [PATCH v4 04/12] ARM: entry: Fix iWMMXT TIF flag handling

[Ard Biesheuvel]

On Tue, 21 Mar 2023 at 20:19, Nicolas Pitre <npitre@baylibre.com> wrote:
>
> On Mon, Mar 20, 2023 at 2:19 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> > The conditional MOVS instruction that appears to have been added to test
> > for the TIF_USING_IWMMXT thread_info flag only sets the N and Z
> > condition flags and register R7, none of which are referenced in the
> > subsequent code.
>
> Really?
>
> As far as I know, the rsb instruction is a "reversed subtract" and that
> also sets the carry flag.
>
> And so does a move with a shifter argument (the last dropped bit is
> moved to the carry flag).
>
> What am I missing?
>

No, you are absolutely right. I looked up the wrong encoding in the
ARM ARM. MOVS without a shift preserves the carry flag, but the
variant you used here behaves as you describe.

So the code is correct - apologies for the noise.