powerpc/signals: Mark VSX not saved with small contexts

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(-)

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

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.