[PATCH] powerpc/signals: Mark VSX not saved with small contexts

[PATCH] powerpc/signals: Mark VSX not saved with small contexts

[Michael Neuling]

The VSX MSR bit in the user context indicates if the context contains VSX
state.  Currently we set this when the process has touched VSX at any stage.

Unfortunately, if the user has not provided enough space to save the VSX state,
we can't save it but we currently still set the MSR VSX bit.

This patch changes this to clear the MSR VSX bit when the user doesn't provide
enough space.  This indicates that there is no valid VSX state in the user
context.

This is needed to support get/set/make/swapcontext for applications that use
VSX but only provide a small context.  For example, getcontext in glibc
provides a smaller context since the VSX registers don't need to be saved over
the glibc function call.  But since the program calling getcontext may have
used VSX, the kernel currently says the VSX state is valid when it's not.  If
the returned context is then used in setcontext (ie. a small context without
VSX but with MSR VSX set), the kernel will refuse the context.  This situation
has been reported by the glibc community.

Based on patch from Carlos O'Donell.

Tested-by: Haren Myneni <haren@linux.vnet.ibm.com>
Signed-off-by: Michael Neuling <mikey@neuling.org>
Cc: stable@vger.kernel.org
---
 arch/powerpc/kernel/signal_32.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 749778e..1844298 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -457,7 +457,15 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
 		if (copy_vsx_to_user(&frame->mc_vsregs, current))
 			return 1;
 		msr |= MSR_VSX;
-	}
+	} else if (!ctx_has_vsx_region)
+		/*
+		 * With a small context structure we can't hold the VSX
+		 * registers, hence clear the MSR value to indicate the state
+		 * was not saved.
+		 */
+		msr &= ~MSR_VSX;
+
+
 #endif /* CONFIG_VSX */
 #ifdef CONFIG_SPE
 	/* save spe registers */
-- 
1.8.3.2

Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

[Michael Ellerman]

On Wed, Nov 20, 2013 at 04:18:54PM +1100, Michael Neuling wrote:
> The VSX MSR bit in the user context indicates if the context contains VSX
> state.  Currently we set this when the process has touched VSX at any stage.
> 
> Unfortunately, if the user has not provided enough space to save the VSX state,
> we can't save it but we currently still set the MSR VSX bit.
> 
> This patch changes this to clear the MSR VSX bit when the user doesn't provide
> enough space.  This indicates that there is no valid VSX state in the user
> context.
> 
> This is needed to support get/set/make/swapcontext for applications that use
> VSX but only provide a small context.  For example, getcontext in glibc
> provides a smaller context since the VSX registers don't need to be saved over
> the glibc function call.  But since the program calling getcontext may have
> used VSX, the kernel currently says the VSX state is valid when it's not.  If
> the returned context is then used in setcontext (ie. a small context without
> VSX but with MSR VSX set), the kernel will refuse the context.  This situation
> has been reported by the glibc community.

Broken since forever?

> Tested-by: Haren Myneni <haren@linux.vnet.ibm.com>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/powerpc/kernel/signal_32.c | 10 +++++++++-

What about the 64-bit code? I don't know the code but it appears at a glance to
have the same bug.


> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 749778e..1844298 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -457,7 +457,15 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
>  		if (copy_vsx_to_user(&frame->mc_vsregs, current))
>  			return 1;
>  		msr |= MSR_VSX;
> -	}
> +	} else if (!ctx_has_vsx_region)
> +		/*
> +		 * With a small context structure we can't hold the VSX
> +		 * registers, hence clear the MSR value to indicate the state
> +		 * was not saved.
> +		 */
> +		msr &= ~MSR_VSX;

I think it'd be clearer if this was just the else case. The full context is:

    if (current->thread.used_vsr && ctx_has_vsx_region) {
            __giveup_vsx(current);
            if (copy_vsx_to_user(&frame->mc_vsregs, current))
                    return 1;
            msr |= MSR_VSX;
+   } else if (!ctx_has_vsx_region)
+           /*
+            * With a small context structure we can't hold the VSX
+            * registers, hence clear the MSR value to indicate the state
+            * was not saved.
+            */
+           msr &= ~MSR_VSX;

Which means if current->thread.user_vsr and ctx_has_vsx_region are both false
we potentially leave MSR_VSX set in msr. I think it should be the case that
MSR_VSX is only ever set if current->thread.used_vsr is true, so it should be
OK in pratice, but it seems unnecessarily fragile.

The logic should be "if we write VSX we set MSR_VSX, else we clear MSR_VSX", ie:

    if (current->thread.used_vsr && ctx_has_vsx_region) {
            __giveup_vsx(current);
            if (copy_vsx_to_user(&frame->mc_vsregs, current))
                    return 1;
            msr |= MSR_VSX;
    } else
            msr &= ~MSR_VSX;

cheers

Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

[Carlos O'Donell]

On 11/21/2013 06:33 AM, Michael Ellerman wrote:
> On Wed, Nov 20, 2013 at 04:18:54PM +1100, Michael Neuling wrote:
>> The VSX MSR bit in the user context indicates if the context contains VSX
>> state.  Currently we set this when the process has touched VSX at any stage.
>>
>> Unfortunately, if the user has not provided enough space to save the VSX state,
>> we can't save it but we currently still set the MSR VSX bit.
>>
>> This patch changes this to clear the MSR VSX bit when the user doesn't provide
>> enough space.  This indicates that there is no valid VSX state in the user
>> context.
>>
>> This is needed to support get/set/make/swapcontext for applications that use
>> VSX but only provide a small context.  For example, getcontext in glibc
>> provides a smaller context since the VSX registers don't need to be saved over
>> the glibc function call.  But since the program calling getcontext may have
>> used VSX, the kernel currently says the VSX state is valid when it's not.  If
>> the returned context is then used in setcontext (ie. a small context without
>> VSX but with MSR VSX set), the kernel will refuse the context.  This situation
>> has been reported by the glibc community.
> 
> Broken since forever?

Yes, broken since forever. At least it was known in glibc 2.18 that this was
broken, but without an active distribution using it the defect wasn't analyzed.

>> Tested-by: Haren Myneni <haren@linux.vnet.ibm.com>
>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>> Cc: stable@vger.kernel.org
>> ---
>>  arch/powerpc/kernel/signal_32.c | 10 +++++++++-
> 
> What about the 64-bit code? I don't know the code but it appears at a glance to
> have the same bug.
 
It doesn't happen with 64-bit code because there the context contains
a sigcontext which on ppc64 has vmx_reserve to store the entire VMX
state. Therefore 64-bit ppc always has space to store the VMX registers
in a userspace context switch. It is only the 32-bit ppc ABI that lacks
the space.
 
>> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
>> index 749778e..1844298 100644
>> --- a/arch/powerpc/kernel/signal_32.c
>> +++ b/arch/powerpc/kernel/signal_32.c
>> @@ -457,7 +457,15 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
>>  		if (copy_vsx_to_user(&frame->mc_vsregs, current))
>>  			return 1;
>>  		msr |= MSR_VSX;
>> -	}
>> +	} else if (!ctx_has_vsx_region)
>> +		/*
>> +		 * With a small context structure we can't hold the VSX
>> +		 * registers, hence clear the MSR value to indicate the state
>> +		 * was not saved.
>> +		 */
>> +		msr &= ~MSR_VSX;
> 
> I think it'd be clearer if this was just the else case. The full context is:
> 
>     if (current->thread.used_vsr && ctx_has_vsx_region) {
>             __giveup_vsx(current);
>             if (copy_vsx_to_user(&frame->mc_vsregs, current))
>                     return 1;
>             msr |= MSR_VSX;
> +   } else if (!ctx_has_vsx_region)
> +           /*
> +            * With a small context structure we can't hold the VSX
> +            * registers, hence clear the MSR value to indicate the state
> +            * was not saved.
> +            */
> +           msr &= ~MSR_VSX;
> 
> Which means if current->thread.user_vsr and ctx_has_vsx_region are both false
> we potentially leave MSR_VSX set in msr. I think it should be the case that
> MSR_VSX is only ever set if current->thread.used_vsr is true, so it should be
> OK in pratice, but it seems unnecessarily fragile.

If current->thread.user_vsr and ctx_has_vsx_region are both false then
!ctx_has_vsx_region is true and we clear MSR_VSX.

Perhaps you mean if current->thread.user_vsr is false, but ctx_has_vsx_region
is true? 

Previously the else clause reset MSR_VSX if:
1. current->thread.used_vsr == 0 && ctx_has_vsx_region == 0
2. current->thread.used_vsr == 1 && ctx_has_vsx_region == 0,

Now it resets MSR_VSX additionally for:
3. current->thread.used_vsr == 0 && ctx_has_vsx_region == 1,

3. is a valid state. The task has not touched the VSX state and the context
is large enough to be saved into. This may be a future state for ppc32 if 
we adjust the ABI to have a large enough context buffer. However at present 
it's not a plausible state. It's also a "don't care" state since there is 
nothing saved in the context, and if nothing was saved in the context then
MSR_VSX is not set.
 
> The logic should be "if we write VSX we set MSR_VSX, else we clear MSR_VSX", ie:
> 
>     if (current->thread.used_vsr && ctx_has_vsx_region) {
>             __giveup_vsx(current);
>             if (copy_vsx_to_user(&frame->mc_vsregs, current))
>                     return 1;
>             msr |= MSR_VSX;
>     } else
>             msr &= ~MSR_VSX;

If anything I dislike this because it might mask a bug in earlier code that
might erroneously set MSR_VSX even though current->thread.user_vsr is not
true. If anything the extra state 3. covered here is a buggy state.

I agree that your suggestion is more robust though since the definition of
robustness is to operate despite failures.

Cheers,
Carlos.

Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

[Michael Neuling]

Carlos O'Donell <carlos@redhat.com> wrote:

> On 11/21/2013 06:33 AM, Michael Ellerman wrote:
> > On Wed, Nov 20, 2013 at 04:18:54PM +1100, Michael Neuling wrote:
> >> The VSX MSR bit in the user context indicates if the context contains VSX
> >> state.  Currently we set this when the process has touched VSX at any stage.
> >>
> >> Unfortunately, if the user has not provided enough space to save the VSX state,
> >> we can't save it but we currently still set the MSR VSX bit.
> >>
> >> This patch changes this to clear the MSR VSX bit when the user doesn't provide
> >> enough space.  This indicates that there is no valid VSX state in the user
> >> context.
> >>
> >> This is needed to support get/set/make/swapcontext for applications that use
> >> VSX but only provide a small context.  For example, getcontext in glibc
> >> provides a smaller context since the VSX registers don't need to be saved over
> >> the glibc function call.  But since the program calling getcontext may have
> >> used VSX, the kernel currently says the VSX state is valid when it's not.  If
> >> the returned context is then used in setcontext (ie. a small context without
> >> VSX but with MSR VSX set), the kernel will refuse the context.  This situation
> >> has been reported by the glibc community.
> > 
> > Broken since forever?
> 
> Yes, broken since forever. At least it was known in glibc 2.18 that this was
> broken, but without an active distribution using it the defect wasn't analyzed.
> 
> >> Tested-by: Haren Myneni <haren@linux.vnet.ibm.com>
> >> Signed-off-by: Michael Neuling <mikey@neuling.org>
> >> Cc: stable@vger.kernel.org
> >> ---
> >>  arch/powerpc/kernel/signal_32.c | 10 +++++++++-
> > 
> > What about the 64-bit code? I don't know the code but it appears at a glance to
> > have the same bug.
>  
> It doesn't happen with 64-bit code because there the context contains
> a sigcontext which on ppc64 has vmx_reserve to store the entire VMX
> state. Therefore 64-bit ppc always has space to store the VMX registers
> in a userspace context switch. It is only the 32-bit ppc ABI that lacks
> the space.

VMX?  I don't understand this at all.  We extended the ucontext to
handle the extra VSX state, so older code may still be using the small
ucontext and we already have a bunch of checks in the 64bit case for
this.

I agree with Michael, we should add this to the 64 bit case.  If we
can't put VSX state in, then clear MSR VSX.

>  
> >> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> >> index 749778e..1844298 100644
> >> --- a/arch/powerpc/kernel/signal_32.c
> >> +++ b/arch/powerpc/kernel/signal_32.c
> >> @@ -457,7 +457,15 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
> >>  		if (copy_vsx_to_user(&frame->mc_vsregs, current))
> >>  			return 1;
> >>  		msr |= MSR_VSX;
> >> -	}
> >> +	} else if (!ctx_has_vsx_region)
> >> +		/*
> >> +		 * With a small context structure we can't hold the VSX
> >> +		 * registers, hence clear the MSR value to indicate the state
> >> +		 * was not saved.
> >> +		 */
> >> +		msr &= ~MSR_VSX;
> > 
> > I think it'd be clearer if this was just the else case. The full context is:
> > 
> >     if (current->thread.used_vsr && ctx_has_vsx_region) {
> >             __giveup_vsx(current);
> >             if (copy_vsx_to_user(&frame->mc_vsregs, current))
> >                     return 1;
> >             msr |= MSR_VSX;
> > +   } else if (!ctx_has_vsx_region)
> > +           /*
> > +            * With a small context structure we can't hold the VSX
> > +            * registers, hence clear the MSR value to indicate the state
> > +            * was not saved.
> > +            */
> > +           msr &= ~MSR_VSX;
> > 
> > Which means if current->thread.user_vsr and ctx_has_vsx_region are both false
> > we potentially leave MSR_VSX set in msr. I think it should be the case that
> > MSR_VSX is only ever set if current->thread.used_vsr is true, so it should be
> > OK in pratice, but it seems unnecessarily fragile.
> 
> If current->thread.user_vsr and ctx_has_vsx_region are both false then
> !ctx_has_vsx_region is true and we clear MSR_VSX.
> 
> Perhaps you mean if current->thread.user_vsr is false, but ctx_has_vsx_region
> is true? 
> 
> Previously the else clause reset MSR_VSX if:
> 1. current->thread.used_vsr == 0 && ctx_has_vsx_region == 0
> 2. current->thread.used_vsr == 1 && ctx_has_vsx_region == 0,
> 
> Now it resets MSR_VSX additionally for:
> 3. current->thread.used_vsr == 0 && ctx_has_vsx_region == 1,
> 
> 3. is a valid state. The task has not touched the VSX state and the context
> is large enough to be saved into. This may be a future state for ppc32 if 
> we adjust the ABI to have a large enough context buffer. However at present 
> it's not a plausible state. It's also a "don't care" state since there is 
> nothing saved in the context, and if nothing was saved in the context then
> MSR_VSX is not set.

This makes my head hurt.

MSR VSX needs to be cleared always if there is no VSX region.  It's
independant of used_vsr, so let's make that clear in the code.

>  
> > The logic should be "if we write VSX we set MSR_VSX, else we clear MSR_VSX", ie:
> > 
> >     if (current->thread.used_vsr && ctx_has_vsx_region) {
> >             __giveup_vsx(current);
> >             if (copy_vsx_to_user(&frame->mc_vsregs, current))
> >                     return 1;
> >             msr |= MSR_VSX;
> >     } else
> >             msr &= ~MSR_VSX;
> 
> If anything I dislike this because it might mask a bug in earlier code that
> might erroneously set MSR_VSX even though current->thread.user_vsr is not
> true. If anything the extra state 3. covered here is a buggy state.
> 
> I agree that your suggestion is more robust though since the definition of
> robustness is to operate despite failures.

The basic idea of the patch is that if the user hasn't passed a VSX
region, then we clear MSR VSX to indicate there is no VSX data.
It's independant of used_vsr.

So how about we make it that simple and put it independent of the other
if statement?

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 749778e..f4a7fd4 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -458,6 +458,14 @@ static int save_user_regs(struct pt_regs *regs, struct mcon
                        return 1;
                msr |= MSR_VSX;
        }
+       /*
+        * With a small context structure we can't hold the VSX registers,
+        * hence clear the MSR value to indicate the state was not saved.
+        */
+       if (!ctx_has_vsx_region)
+               msr &= ~MSR_VSX;
+
+
 #endif /* CONFIG_VSX */
 #ifdef CONFIG_SPE
        /* save spe registers */

Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

[Carlos O'Donell]

On 11/21/2013 05:21 PM, Michael Neuling wrote:
>>> What about the 64-bit code? I don't know the code but it appears at a glance to
>>> have the same bug.
>>  
>> It doesn't happen with 64-bit code because there the context contains
>> a sigcontext which on ppc64 has vmx_reserve to store the entire VMX
>> state. Therefore 64-bit ppc always has space to store the VMX registers
>> in a userspace context switch. It is only the 32-bit ppc ABI that lacks
>> the space.
> 
> VMX?  I don't understand this at all.  We extended the ucontext to
> handle the extra VSX state, so older code may still be using the small
> ucontext and we already have a bunch of checks in the 64bit case for
> this.
> 
> I agree with Michael, we should add this to the 64 bit case.  If we
> can't put VSX state in, then clear MSR VSX.

Sorry, typo, VSX not VMX.

I had not gone through the historical implementation of the 64-bit
code, I assumed it started with a sufficiently sized context structure,
but on closer review it didn't.

The addition of the *context functions in glibc for 64-bit power
happened in 2003 by glibc commit 609b4783, with the mcontext_t
being expanded to include 

I see that the 64-bit userspace context was extended in 2008 by your 
kernel commit ce48b210.

Thus you're right the check is needed in the 64-bit case.

However, at present the issue doesn't seem to trigger in the
64-bit userspace. Which is odd now that I review the code and 
see that the 64-bit userspace context is smaller than the kernel
context (lacks the `+32' to the vmx_reserve space). It could just
be that the compiler finds no chance to use VSX and therefore
the existing test cases don't trigger the bug. I don't plan to
investigate this further given that we're going to fix the 64-bit case
also.
 
> So how about we make it that simple and put it independent of the other
> if statement?
> 
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 749778e..f4a7fd4 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -458,6 +458,14 @@ static int save_user_regs(struct pt_regs *regs, struct mcon
>                         return 1;
>                 msr |= MSR_VSX;
>         }
> +       /*
> +        * With a small context structure we can't hold the VSX registers,
> +        * hence clear the MSR value to indicate the state was not saved.
> +        */
> +       if (!ctx_has_vsx_region)
> +               msr &= ~MSR_VSX;
> +
> +
>  #endif /* CONFIG_VSX */
>  #ifdef CONFIG_SPE
>         /* save spe registers */
> 

Looks good to me, along with a similar fix for signal_64.c.

Cheers,
Carlos.

Re: [PATCH] powerpc/signals: Mark VSX not saved with small contexts

[Carlos O'Donell]

On 11/21/2013 07:53 PM, Carlos O'Donell wrote:
> The addition of the *context functions in glibc for 64-bit power
> happened in 2003 by glibc commit 609b4783, with the mcontext_t
> being expanded to include 

... support for VMX state via `long vmx_reserve[NVRREG+NVRREG+1];'.

Cheers,
Carlos.

[PATCH v2] powerpc/signals: Mark VSX not saved with small contexts

[Michael Neuling]

The VSX MSR bit in the user context indicates if the context contains
VSX state.  Unfortunately, if the user has not provided enough space to
save the VSX state, we can't save it but we currently still set the MSR
VSX bit.

This patch changes this to clear the MSR VSX bit when the user doesn't
provide enough space.  This indicates that there is no valid VSX state
in the user context.  We now clear MSR VSX always and only set it in the
specific case when we can (ie. when VSX used and space is provided).

This is needed to support get/set/make/swapcontext for applications that
use VSX but only provide a small context.  For example, getcontext in
glibc provides a smaller context since the VSX registers don't need to
be saved over the glibc function call.  But since the program calling
getcontext may have used VSX, the kernel currently says the VSX state is
valid when it's not.  If the returned context is then used in setcontext
(ie. a small context without VSX but with MSR VSX set), the kernel will
refuse the context.  This situation has been reported by the glibc
community.

Based on patch from Carlos O'Donell.

Tested-by: Haren Myneni <haren@linux.vnet.ibm.com>
Signed-off-by: Michael Neuling <mikey@neuling.org>
Cc: stable@vger.kernel.org
--
v2:
  - moved the code around a bit to make clearer what's happening
  - added 64bit version as noticed by mpe
  - updates to comments and commit messages

diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 749778e..68027bf 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -445,6 +445,12 @@ static int save_user_regs(struct pt_regs *regs, struct mcontext __user *frame,
 #endif /* CONFIG_ALTIVEC */
 	if (copy_fpr_to_user(&frame->mc_fregs, current))
 		return 1;
+
+	/*
+	 * Clear the MSR VSX bit to indicate there is no valid state attached
+	 * to this context, except in the specific case below where we set it.
+	 */
+	msr &= ~MSR_VSX;
 #ifdef CONFIG_VSX
 	/*
 	 * Copy VSR 0-31 upper half from thread_struct to local
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index b3c6157..26789ed 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -122,6 +122,12 @@ static long setup_sigcontext(struct sigcontext __user *sc, struct pt_regs *regs,
 	flush_fp_to_thread(current);
 	/* copy fpr regs and fpscr */
 	err |= copy_fpr_to_user(&sc->fp_regs, current);
+
+	/*
+	 * Clear the MSR VSX bit to indicate there is no valid state attached
+	 * to this context, except in the specific case below where we set it.
+	 */
+	msr &= ~MSR_VSX;
 #ifdef CONFIG_VSX
 	/*
 	 * Copy VSX low doubleword to local buffer for formatting,