iwmmxt signal frame handling

iwmmxt signal frame handling

[Arnd Bergmann]

Peter Maydell noticed during code review that the signal frame might
be written incorrectly for kernels with CONFIG_IWMMXT set running
processes without TIF_USING_IWMMXT, where the magic/size values
for the iwmmxt section of the frame are left uninitialized.
Instead of skipping this part of the frame, we should instead
write a valid header with zero data.

This patch is compile-tested only since the problem was only
found in review and neither Peter nor myself have access to
IWMMXT capable hardware to test this on.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reported-by: Peter Maydell <peter.maydell@linaro.org>

--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -154,7 +154,10 @@ static int preserve_iwmmxt_context(struct iwmmxt_sigframe *frame)
 	kframe = (struct iwmmxt_sigframe *)((unsigned long)(kbuf + 8) & ~7);
 	kframe->magic = IWMMXT_MAGIC;
 	kframe->size = IWMMXT_STORAGE_SIZE;
-	iwmmxt_task_copy(current_thread_info(), &kframe->storage);
+	if (test_thread_flag(TIF_USING_IWMMXT))
+		iwmmxt_task_copy(current_thread_info(), &kframe->storage);
+	else
+		memset(&kframe->storage, 0, sizeof(kframe->storage));
 	return __copy_to_user(frame, kframe, sizeof(*frame));
 }
 
@@ -429,7 +432,7 @@ setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
 		err |= preserve_crunch_context(&aux->crunch);
 #endif
 #ifdef CONFIG_IWMMXT
-	if (err == 0 && test_thread_flag(TIF_USING_IWMMXT))
+	if (err == 0)
 		err |= preserve_iwmmxt_context(&aux->iwmmxt);
 #endif
 #ifdef CONFIG_VFP

iwmmxt signal frame handling

[Nicolas Pitre]

On Thu, 14 Oct 2010, Arnd Bergmann wrote:

> Peter Maydell noticed during code review that the signal frame might
> be written incorrectly for kernels with CONFIG_IWMMXT set running
> processes without TIF_USING_IWMMXT, where the magic/size values
> for the iwmmxt section of the frame are left uninitialized.
> Instead of skipping this part of the frame, we should instead
> write a valid header with zero data.
> 
> This patch is compile-tested only since the problem was only
> found in review and neither Peter nor myself have access to
> IWMMXT capable hardware to test this on.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>



> 
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -154,7 +154,10 @@ static int preserve_iwmmxt_context(struct iwmmxt_sigframe *frame)
>  	kframe = (struct iwmmxt_sigframe *)((unsigned long)(kbuf + 8) & ~7);
>  	kframe->magic = IWMMXT_MAGIC;
>  	kframe->size = IWMMXT_STORAGE_SIZE;
> -	iwmmxt_task_copy(current_thread_info(), &kframe->storage);
> +	if (test_thread_flag(TIF_USING_IWMMXT))
> +		iwmmxt_task_copy(current_thread_info(), &kframe->storage);
> +	else
> +		memset(&kframe->storage, 0, sizeof(kframe->storage));
>  	return __copy_to_user(frame, kframe, sizeof(*frame));
>  }
>  
> @@ -429,7 +432,7 @@ setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
>  		err |= preserve_crunch_context(&aux->crunch);
>  #endif
>  #ifdef CONFIG_IWMMXT
> -	if (err == 0 && test_thread_flag(TIF_USING_IWMMXT))
> +	if (err == 0)
>  		err |= preserve_iwmmxt_context(&aux->iwmmxt);
>  #endif
>  #ifdef CONFIG_VFP
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

iwmmxt signal frame handling

[Eric Miao]

On Thu, Oct 14, 2010 at 10:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> Peter Maydell noticed during code review that the signal frame might
> be written incorrectly for kernels with CONFIG_IWMMXT set running
> processes without TIF_USING_IWMMXT, where the magic/size values
> for the iwmmxt section of the frame are left uninitialized.
> Instead of skipping this part of the frame, we should instead
> write a valid header with zero data.
>
> This patch is compile-tested only since the problem was only
> found in review and neither Peter nor myself have access to
> IWMMXT capable hardware to test this on.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Reported-by: Peter Maydell <peter.maydell@linaro.org>

Looks OK to me as well.

Acked-by: Eric Miao <eric.y.miao@gmail.com>

>
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -154,7 +154,10 @@ static int preserve_iwmmxt_context(struct iwmmxt_sigframe *frame)
> ? ? ? ?kframe = (struct iwmmxt_sigframe *)((unsigned long)(kbuf + 8) & ~7);
> ? ? ? ?kframe->magic = IWMMXT_MAGIC;
> ? ? ? ?kframe->size = IWMMXT_STORAGE_SIZE;
> - ? ? ? iwmmxt_task_copy(current_thread_info(), &kframe->storage);
> + ? ? ? if (test_thread_flag(TIF_USING_IWMMXT))
> + ? ? ? ? ? ? ? iwmmxt_task_copy(current_thread_info(), &kframe->storage);
> + ? ? ? else
> + ? ? ? ? ? ? ? memset(&kframe->storage, 0, sizeof(kframe->storage));
> ? ? ? ?return __copy_to_user(frame, kframe, sizeof(*frame));
> ?}
>
> @@ -429,7 +432,7 @@ setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
> ? ? ? ? ? ? ? ?err |= preserve_crunch_context(&aux->crunch);
> ?#endif
> ?#ifdef CONFIG_IWMMXT
> - ? ? ? if (err == 0 && test_thread_flag(TIF_USING_IWMMXT))
> + ? ? ? if (err == 0)
> ? ? ? ? ? ? ? ?err |= preserve_iwmmxt_context(&aux->iwmmxt);
> ?#endif
> ?#ifdef CONFIG_VFP
>

iwmmxt signal frame handling

[Eric Miao]

On Friday, October 15, 2010, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Thu, Oct 14, 2010 at 10:09 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> Peter Maydell noticed during code review that the signal frame might
>> be written incorrectly for kernels with CONFIG_IWMMXT set running
>> processes without TIF_USING_IWMMXT, where the magic/size values
>> for the iwmmxt section of the frame are left uninitialized.
>> Instead of skipping this part of the frame, we should instead
>> write a valid header with zero data.
>>
>> This patch is compile-tested only since the problem was only
>> found in review and neither Peter nor myself have access to
>> IWMMXT capable hardware to test this on.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>
> Looks OK to me as well.
>
> Acked-by: Eric Miao <eric.y.miao@gmail.com>
>

This also passes the internal iwmmxt test, Haojian please add your
Tested-by please.

>>
>> --- a/arch/arm/kernel/signal.c
>> +++ b/arch/arm/kernel/signal.c
>> @@ -154,7 +154,10 @@ static int preserve_iwmmxt_context(struct iwmmxt_sigframe *frame)
>> ? ? ? ?kframe = (struct iwmmxt_sigframe *)((unsigned long)(kbuf + 8) & ~7);
>> ? ? ? ?kframe->magic = IWMMXT_MAGIC;
>> ? ? ? ?kframe->size = IWMMXT_STORAGE_SIZE;
>> - ? ? ? iwmmxt_task_copy(current_thread_info(), &kframe->storage);
>> + ? ? ? if (test_thread_flag(TIF_USING_IWMMXT))
>> + ? ? ? ? ? ? ? iwmmxt_task_copy(current_thread_info(), &kframe->storage);
>> + ? ? ? else
>> + ? ? ? ? ? ? ? memset(&kframe->storage, 0, sizeof(kframe->storage));
>> ? ? ? ?return __copy_to_user(frame, kframe, sizeof(*frame));
>> ?}
>>
>> @@ -429,7 +432,7 @@ setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
>> ? ? ? ? ? ? ? ?err |= preserve_crunch_context(&aux->crunch);
>> ?#endif
>> ?#ifdef CONFIG_IWMMXT
>> - ? ? ? if (err == 0 && test_thread_flag(TIF_USING_IWMMXT))
>> + ? ? ? if (err == 0)
>> ? ? ? ? ? ? ? ?err |= preserve_iwmmxt_context(&aux->iwmmxt);
>> ?#endif
>> ?#ifdef CONFIG_VFP
>>
>

iwmmxt signal frame handling

[Arnd Bergmann]

On Friday 15 October 2010 15:32:04 Eric Miao wrote:
> > Acked-by: Eric Miao <eric.y.miao@gmail.com>
> >
> 
> This also passes the internal iwmmxt test, Haojian please add your
> Tested-by please.

Can you also confirm that the previous behaviour is broken?
If it is, the patch is probably a candidate for the stable trees.

	Arnd

iwmmxt signal frame handling

[Eric Miao]

On Fri, Oct 15, 2010 at 9:40 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 15 October 2010 15:32:04 Eric Miao wrote:
>> > Acked-by: Eric Miao <eric.y.miao@gmail.com>
>> >
>>
>> This also passes the internal iwmmxt test, Haojian please add your
>> Tested-by please.
>
> Can you also confirm that the previous behaviour is broken?
> If it is, the patch is probably a candidate for the stable trees.
>

I don't believe the previous behavior is going to cause any serious issue. The
magic/size are only saved/restored only for those processes with
TIF_USING_IWMMXT
flag set. Yet this patch does seem to improve from security perspective by
zero-ing the buffer.

BTW: I didn't look too much into the code, but it seems to me that the aux can
just be zero-ed in a whole?


> ? ? ? ?Arnd
>

iwmmxt signal frame handling

[Nicolas Pitre]

On Fri, 15 Oct 2010, Eric Miao wrote:

> On Fri, Oct 15, 2010 at 9:40 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 15 October 2010 15:32:04 Eric Miao wrote:
> >> > Acked-by: Eric Miao <eric.y.miao@gmail.com>
> >> >
> >>
> >> This also passes the internal iwmmxt test, Haojian please add your
> >> Tested-by please.
> >
> > Can you also confirm that the previous behaviour is broken?
> > If it is, the patch is probably a candidate for the stable trees.
> >
> 
> I don't believe the previous behavior is going to cause any serious issue. The
> magic/size are only saved/restored only for those processes with
> TIF_USING_IWMMXT
> flag set. Yet this patch does seem to improve from security perspective by
> zero-ing the buffer.
> 
> BTW: I didn't look too much into the code, but it seems to me that the aux can
> just be zero-ed in a whole?

No.  Extensions to the ucontext layout are structured into:

	- magic number to identify the data

	- size of the data

	- the data itself

and you may have multiple of those blocks together, with the last one 
having a magic number of 0 to mark the end.

What could be done instead is to avoid copying any iwmmxt context when 
that is not available (i.e. when TIF_USING_IWMMXT is not set), but that 
would make the code more complex as the ucontext structure couldn't be 
used as is and some kind of dynamic storage woule be needed.  I really 
doubt it is worth the trouble, especially since the only environment 
where that would make a difference is with those old pre-EABI binaries 
which don't care at all about those ucontext extensions anyway.


Nicolas

iwmmxt signal frame handling

[Haojian Zhuang]

>-----Original Message-----
>From: Eric Miao [mailto:eric.y.miao at gmail.com]
>Sent: 2010?10?15? 11:19 PM
>To: Arnd Bergmann
>Cc: linux-arm-kernel at lists.infradead.org; Peter Maydell; Russell King; Haojian Zhuang
>Subject: Re: iwmmxt signal frame handling
>
>On Fri, Oct 15, 2010 at 9:40 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Friday 15 October 2010 15:32:04 Eric Miao wrote:
>>> > Acked-by: Eric Miao <eric.y.miao@gmail.com>
>>> >
>>>
>>> This also passes the internal iwmmxt test, Haojian please add your
>>> Tested-by please.
>>
>> Can you also confirm that the previous behaviour is broken?
>> If it is, the patch is probably a candidate for the stable trees.
>>
>
>I don't believe the previous behavior is going to cause any serious issue. The
>magic/size are only saved/restored only for those processes with
>TIF_USING_IWMMXT
>flag set. Yet this patch does seem to improve from security perspective by
>zero-ing the buffer.
>
>BTW: I didn't look too much into the code, but it seems to me that the aux can
>just be zero-ed in a whole?
>
>

Yes, the previous behavior doesn't cause serious issue on the testcases.

You can add "Test-by: Haojian Zhuang <haojian.zhuang@marvell.com>".

Thanks
Haojian

iwmmxt signal frame handling

[Nicolas Pitre]

On Fri, 15 Oct 2010, Arnd Bergmann wrote:

> On Friday 15 October 2010 15:32:04 Eric Miao wrote:
> > > Acked-by: Eric Miao <eric.y.miao@gmail.com>
> > >
> > 
> > This also passes the internal iwmmxt test, Haojian please add your
> > Tested-by please.
> 
> Can you also confirm that the previous behaviour is broken?
> If it is, the patch is probably a candidate for the stable trees.

Hard to confirm.  The only way not to have TIF_USING_IWMMXT set is to 
use old pre-EABI binaries, which are relying on the kernel emulated FPA 
instructions for floating point, and nothing in those days might have 
cared about extensions to the ucontext layout at all, or even have used 
iwmmxt.  While this is a correctness fix, in practice this never 
mattered.

And we're talking about mixed ABI environments which existed many years 
ago, like prior the Git era.  Hardly a stable tree candidate.


Nicolas

iwmmxt signal frame handling

[Arnd Bergmann]

On Friday 15 October 2010 20:38:15 Nicolas Pitre wrote:
> Hard to confirm.  The only way not to have TIF_USING_IWMMXT set is to 
> use old pre-EABI binaries, which are relying on the kernel emulated FPA 
> instructions for floating point, and nothing in those days might have 
> cared about extensions to the ucontext layout at all, or even have used 
> iwmmxt.  While this is a correctness fix, in practice this never 
> mattered.

Ok, good to know.
 
> And we're talking about mixed ABI environments which existed many years 
> ago, like prior the Git era.  Hardly a stable tree candidate.

Yes, that's pretty clear then. I had assumed the TIF flag meant that
a particular process had been using IWMMXT, not that it was allowed
to do them.

Thanks for the explanation,

	Arnd