[PATCH] Fix Warnining in arch/x86/kernel/signal.c

[PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Subrata Modak]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 3291 bytes --]

Hi,

>On Tue, 2009-05-12 at 17:16 +0200, Andi Kleen wrote:
>On Tue, May 12, 2009 at 05:16:14PM +0530, Subrata Modak wrote:
> 
> Hi Subrata,
> 
> > 
> > With gcc (GCC) 4.4.1 20090429 (prerelease), i get the following build warning:
> 
> Patch looks good (you can add a 
> Reviewed-by: Andi Kleen <ak@linux.intel.com>)
> But I don't maintain this code anymore. Please resend to x86@kernel.org
> cc linux-kernel@vger.kernel.org for merge.
> 
> Thanks,
> 
> -Andi


With gcc (GCC) 4.4.1 20090429 (prerelease), i get the following build warning:

CC      arch/x86/kernel/signal.o
arch/x86/kernel/signal.c: In function ‘sys_sigreturn’:
arch/x86/kernel/signal.c:573: warning: ‘set.sig[1]’ may be used uninitialized in this function

On investigation i found that this is because of the evaluation
precedence of the expression below:

569 unsigned long sys_sigreturn(struct pt_regs *regs)
570 {
571         struct sigframe __user *frame;
572         unsigned long ax;
573         sigset_t set;
574 
575         frame = (struct sigframe __user *)(regs->sp - 8);
576 
577         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
578                 goto badframe;
579         if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
580                 && __copy_from_user(&set.sig[1], &frame->extramask,
581                                     sizeof(frame->extramask))))

The initialization for set.sig[1] may not occur if
	__get_user(set.sig[0], &frame->sc.oldmask)
evalutes to true. So, the compiler is complaining.

I have devised a small patch for this which wanes away this warning
without changing the conditional evaluation criteria. Let me know if
you like this patch.

582                 goto badframe;
583 
584         sigdelsetmask(&set, ~_BLOCKABLE);
585         spin_lock_irq(&current->sighand->siglock);
586         current->blocked = set;
587         recalc_sigpending();
588         spin_unlock_irq(&current->sighand->siglock);
589 
590         if (restore_sigcontext(regs, &frame->sc, &ax))
591                 goto badframe;
592         return ax;
593 
594 badframe:
595         signal_fault(regs, frame, "sigreturn");
596 
597         return 0;
598 }

Signed-Off-By: Subrata Modak <subrata@linux.vnet.ibm.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
To: <x86@kernel.org>
Cc: Linux Kernel <linux-kernel@vger.kernel.org>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
Cc: Sachin P Sant <sachinp@linux.vnet.ibm.com>
Subject: [PATCH] Fix Warnining in arch/x86/kernel/signal.c
---

--- a/arch/x86/kernel/signal.c	2009-05-12 10:59:24.000000000 +0530
+++ b/arch/x86/kernel/signal.c	2009-05-12 16:57:32.000000000 +0530
@@ -576,9 +576,10 @@ unsigned long sys_sigreturn(struct pt_re
 
 	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
 		goto badframe;
-	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
-		&& __copy_from_user(&set.sig[1], &frame->extramask,
-				    sizeof(frame->extramask))))
+
+        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
+                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
+                __get_user(set.sig[0], &frame->sc.oldmask))
 		goto badframe;
 
 	sigdelsetmask(&set, ~_BLOCKABLE);

---
Regards--
Subrata

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Hiroshi Shimamoto]

Subrata Modak wrote:
> Hi,
> 
>> On Tue, 2009-05-12 at 17:16 +0200, Andi Kleen wrote:
>> On Tue, May 12, 2009 at 05:16:14PM +0530, Subrata Modak wrote:
>>
>> Hi Subrata,
>>
>>> With gcc (GCC) 4.4.1 20090429 (prerelease), i get the following build warning:
>> Patch looks good (you can add a 
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>)
>> But I don't maintain this code anymore. Please resend to x86@kernel.org
>> cc linux-kernel@vger.kernel.org for merge.
>>
>> Thanks,
>>
>> -Andi
> 
> 
> With gcc (GCC) 4.4.1 20090429 (prerelease), i get the following build warning:
> 
> CC      arch/x86/kernel/signal.o
> arch/x86/kernel/signal.c: In function ‘sys_sigreturn’:
> arch/x86/kernel/signal.c:573: warning: ‘set.sig[1]’ may be used uninitialized in this function
> 
> On investigation i found that this is because of the evaluation
> precedence of the expression below:
> 
> 569 unsigned long sys_sigreturn(struct pt_regs *regs)
> 570 {
> 571         struct sigframe __user *frame;
> 572         unsigned long ax;
> 573         sigset_t set;
> 574 
> 575         frame = (struct sigframe __user *)(regs->sp - 8);
> 576 
> 577         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
> 578                 goto badframe;
> 579         if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> 580                 && __copy_from_user(&set.sig[1], &frame->extramask,
> 581                                     sizeof(frame->extramask))))
> 
> The initialization for set.sig[1] may not occur if
> 	__get_user(set.sig[0], &frame->sc.oldmask)
> evalutes to true. So, the compiler is complaining.
> 
> I have devised a small patch for this which wanes away this warning
> without changing the conditional evaluation criteria. Let me know if
> you like this patch.
> 
> 582                 goto badframe;
> 583 
> 584         sigdelsetmask(&set, ~_BLOCKABLE);
> 585         spin_lock_irq(&current->sighand->siglock);
> 586         current->blocked = set;
> 587         recalc_sigpending();
> 588         spin_unlock_irq(&current->sighand->siglock);
> 589 
> 590         if (restore_sigcontext(regs, &frame->sc, &ax))
> 591                 goto badframe;
> 592         return ax;
> 593 
> 594 badframe:
> 595         signal_fault(regs, frame, "sigreturn");
> 596 
> 597         return 0;
> 598 }
> 
> Signed-Off-By: Subrata Modak <subrata@linux.vnet.ibm.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> To: <x86@kernel.org>
> Cc: Linux Kernel <linux-kernel@vger.kernel.org>
> Cc: Andi Kleen <andi@firstfloor.org>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> Cc: Sachin P Sant <sachinp@linux.vnet.ibm.com>
> Subject: [PATCH] Fix Warnining in arch/x86/kernel/signal.c
> ---
> 
> --- a/arch/x86/kernel/signal.c	2009-05-12 10:59:24.000000000 +0530
> +++ b/arch/x86/kernel/signal.c	2009-05-12 16:57:32.000000000 +0530
> @@ -576,9 +576,10 @@ unsigned long sys_sigreturn(struct pt_re
>  
>  	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>  		goto badframe;
> -	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> -		&& __copy_from_user(&set.sig[1], &frame->extramask,
> -				    sizeof(frame->extramask))))
> +
> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
> +                __get_user(set.sig[0], &frame->sc.oldmask))
>  		goto badframe;

I'm not sure why this eliminates that warning.
set.sig[0] may not be initialized too, if __copy_from_user() failed.
I don't have enough time to look at this right now, sorry.

Another question, __copy_from_user() will be called even if
_NSIG_WORDS is less than 2, perhaps it never occurs.
I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
is better.

Thanks,
Hiroshi

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Subrata Modak]

On Wed, 2009-05-13 at 11:20 +0900, Hiroshi Shimamoto wrote:
> Subrata Modak wrote:
> > Hi,
> > 
> >> On Tue, 2009-05-12 at 17:16 +0200, Andi Kleen wrote:
> >> On Tue, May 12, 2009 at 05:16:14PM +0530, Subrata Modak wrote:
> >>
> >> Hi Subrata,
> >>
> >>> With gcc (GCC) 4.4.1 20090429 (prerelease), i get the following build warning:
> >> Patch looks good (you can add a 
> >> Reviewed-by: Andi Kleen <ak@linux.intel.com>)
> >> But I don't maintain this code anymore. Please resend to x86@kernel.org
> >> cc linux-kernel@vger.kernel.org for merge.
> >>
> >> Thanks,
> >>
> >> -Andi
> > 
> > 
> > With gcc (GCC) 4.4.1 20090429 (prerelease), i get the following build warning:
> > 
> > CC      arch/x86/kernel/signal.o
> > arch/x86/kernel/signal.c: In function ‘sys_sigreturn’:
> > arch/x86/kernel/signal.c:573: warning: ‘set.sig[1]’ may be used uninitialized in this function
> > 
> > On investigation i found that this is because of the evaluation
> > precedence of the expression below:
> > 
> > 569 unsigned long sys_sigreturn(struct pt_regs *regs)
> > 570 {
> > 571         struct sigframe __user *frame;
> > 572         unsigned long ax;
> > 573         sigset_t set;
> > 574 
> > 575         frame = (struct sigframe __user *)(regs->sp - 8);
> > 576 
> > 577         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
> > 578                 goto badframe;
> > 579         if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> > 580                 && __copy_from_user(&set.sig[1], &frame->extramask,
> > 581                                     sizeof(frame->extramask))))
> > 
> > The initialization for set.sig[1] may not occur if
> > 	__get_user(set.sig[0], &frame->sc.oldmask)
> > evalutes to true. So, the compiler is complaining.
> > 
> > I have devised a small patch for this which wanes away this warning
> > without changing the conditional evaluation criteria. Let me know if
> > you like this patch.
> > 
> > 582                 goto badframe;
> > 583 
> > 584         sigdelsetmask(&set, ~_BLOCKABLE);
> > 585         spin_lock_irq(&current->sighand->siglock);
> > 586         current->blocked = set;
> > 587         recalc_sigpending();
> > 588         spin_unlock_irq(&current->sighand->siglock);
> > 589 
> > 590         if (restore_sigcontext(regs, &frame->sc, &ax))
> > 591                 goto badframe;
> > 592         return ax;
> > 593 
> > 594 badframe:
> > 595         signal_fault(regs, frame, "sigreturn");
> > 596 
> > 597         return 0;
> > 598 }
> > 
> > Signed-Off-By: Subrata Modak <subrata@linux.vnet.ibm.com>
> > Reviewed-by: Andi Kleen <ak@linux.intel.com>
> > To: <x86@kernel.org>
> > Cc: Linux Kernel <linux-kernel@vger.kernel.org>
> > Cc: Andi Kleen <andi@firstfloor.org>
> > Cc: Andi Kleen <ak@linux.intel.com>
> > Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> > Cc: Sachin P Sant <sachinp@linux.vnet.ibm.com>
> > Subject: [PATCH] Fix Warnining in arch/x86/kernel/signal.c
> > ---
> > 
> > --- a/arch/x86/kernel/signal.c	2009-05-12 10:59:24.000000000 +0530
> > +++ b/arch/x86/kernel/signal.c	2009-05-12 16:57:32.000000000 +0530
> > @@ -576,9 +576,10 @@ unsigned long sys_sigreturn(struct pt_re
> >  
> >  	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
> >  		goto badframe;
> > -	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> > -		&& __copy_from_user(&set.sig[1], &frame->extramask,
> > -				    sizeof(frame->extramask))))
> > +
> > +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
> > +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
> > +                __get_user(set.sig[0], &frame->sc.oldmask))
> >  		goto badframe;
> 
> I'm not sure why this eliminates that warning.
> set.sig[0] may not be initialized too, if __copy_from_user() failed.

True, but only when either or both of __copy_from_user() and
(_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
initialized.

> I don't have enough time to look at this right now, sorry.
> 
> Another question, __copy_from_user() will be called even if
> _NSIG_WORDS is less than 2, perhaps it never occurs.
> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
> is better.

Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
drop it.

Regards--
Subrata

> 
> Thanks,
> Hiroshi
> 

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Ingo Molnar]

* Subrata Modak <subrata@linux.vnet.ibm.com> wrote:

> On Wed, 2009-05-13 at 11:20 +0900, Hiroshi Shimamoto wrote:
> > Subrata Modak wrote:
> > > Hi,
> > > 
> > >> On Tue, 2009-05-12 at 17:16 +0200, Andi Kleen wrote:
> > >> On Tue, May 12, 2009 at 05:16:14PM +0530, Subrata Modak wrote:
> > >>
> > >> Hi Subrata,
> > >>
> > >>> With gcc (GCC) 4.4.1 20090429 (prerelease), i get the following build warning:
> > >> Patch looks good (you can add a 
> > >> Reviewed-by: Andi Kleen <ak@linux.intel.com>)
> > >> But I don't maintain this code anymore. Please resend to x86@kernel.org
> > >> cc linux-kernel@vger.kernel.org for merge.
> > >>
> > >> Thanks,
> > >>
> > >> -Andi
> > > 
> > > 
> > > With gcc (GCC) 4.4.1 20090429 (prerelease), i get the following build warning:
> > > 
> > > CC      arch/x86/kernel/signal.o
> > > arch/x86/kernel/signal.c: In function ‘sys_sigreturn’:
> > > arch/x86/kernel/signal.c:573: warning: ‘set.sig[1]’ may be used uninitialized in this function
> > > 
> > > On investigation i found that this is because of the evaluation
> > > precedence of the expression below:
> > > 
> > > 569 unsigned long sys_sigreturn(struct pt_regs *regs)
> > > 570 {
> > > 571         struct sigframe __user *frame;
> > > 572         unsigned long ax;
> > > 573         sigset_t set;
> > > 574 
> > > 575         frame = (struct sigframe __user *)(regs->sp - 8);
> > > 576 
> > > 577         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
> > > 578                 goto badframe;
> > > 579         if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> > > 580                 && __copy_from_user(&set.sig[1], &frame->extramask,
> > > 581                                     sizeof(frame->extramask))))
> > > 
> > > The initialization for set.sig[1] may not occur if
> > > 	__get_user(set.sig[0], &frame->sc.oldmask)
> > > evalutes to true. So, the compiler is complaining.
> > > 
> > > I have devised a small patch for this which wanes away this warning
> > > without changing the conditional evaluation criteria. Let me know if
> > > you like this patch.
> > > 
> > > 582                 goto badframe;
> > > 583 
> > > 584         sigdelsetmask(&set, ~_BLOCKABLE);
> > > 585         spin_lock_irq(&current->sighand->siglock);
> > > 586         current->blocked = set;
> > > 587         recalc_sigpending();
> > > 588         spin_unlock_irq(&current->sighand->siglock);
> > > 589 
> > > 590         if (restore_sigcontext(regs, &frame->sc, &ax))
> > > 591                 goto badframe;
> > > 592         return ax;
> > > 593 
> > > 594 badframe:
> > > 595         signal_fault(regs, frame, "sigreturn");
> > > 596 
> > > 597         return 0;
> > > 598 }
> > > 
> > > Signed-Off-By: Subrata Modak <subrata@linux.vnet.ibm.com>
> > > Reviewed-by: Andi Kleen <ak@linux.intel.com>
> > > To: <x86@kernel.org>
> > > Cc: Linux Kernel <linux-kernel@vger.kernel.org>
> > > Cc: Andi Kleen <andi@firstfloor.org>
> > > Cc: Andi Kleen <ak@linux.intel.com>
> > > Cc: Balbir Singh <balbir@linux.vnet.ibm.com>
> > > Cc: Sachin P Sant <sachinp@linux.vnet.ibm.com>
> > > Subject: [PATCH] Fix Warnining in arch/x86/kernel/signal.c
> > > ---
> > > 
> > > --- a/arch/x86/kernel/signal.c	2009-05-12 10:59:24.000000000 +0530
> > > +++ b/arch/x86/kernel/signal.c	2009-05-12 16:57:32.000000000 +0530
> > > @@ -576,9 +576,10 @@ unsigned long sys_sigreturn(struct pt_re
> > >  
> > >  	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
> > >  		goto badframe;
> > > -	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> > > -		&& __copy_from_user(&set.sig[1], &frame->extramask,
> > > -				    sizeof(frame->extramask))))
> > > +
> > > +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
> > > +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
> > > +                __get_user(set.sig[0], &frame->sc.oldmask))
> > >  		goto badframe;
> > 
> > I'm not sure why this eliminates that warning.
> > set.sig[0] may not be initialized too, if __copy_from_user() failed.
> 
> True, but only when either or both of __copy_from_user() and
> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
> initialized.
> 
> > I don't have enough time to look at this right now, sorry.
> > 
> > Another question, __copy_from_user() will be called even if
> > _NSIG_WORDS is less than 2, perhaps it never occurs.
> > I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
> > is better.
> 
> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
> drop it.

If you get the Acked-by from Hiroshi-san it looks good to me. He 
modified this code last.

	Ingo

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[H. Peter Anvin]

Ingo Molnar wrote:
>>>>  
>>>>  	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>>>  		goto badframe;
>>>> -	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
>>>> -		&& __copy_from_user(&set.sig[1], &frame->extramask,
>>>> -				    sizeof(frame->extramask))))
>>>> +
>>>> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
>>>> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
>>>> +                __get_user(set.sig[0], &frame->sc.oldmask))
>>>>  		goto badframe;
>>> I'm not sure why this eliminates that warning.
>>> set.sig[0] may not be initialized too, if __copy_from_user() failed.
>> True, but only when either or both of __copy_from_user() and
>> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
>> initialized.
>>
>>> I don't have enough time to look at this right now, sorry.
>>>
>>> Another question, __copy_from_user() will be called even if
>>> _NSIG_WORDS is less than 2, perhaps it never occurs.
>>> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
>>> is better.
>> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
>> drop it.
> 
> If you get the Acked-by from Hiroshi-san it looks good to me. He 
> modified this code last.
> 

This seriously looks wrong to me.  If _NSIG_WORDS == 1, then calling
__copy_from_user here is a serious error.

	-hpa

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Hiroshi Shimamoto]

H. Peter Anvin wrote:
> Ingo Molnar wrote:
>>>>>  
>>>>>  	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>>>>  		goto badframe;
>>>>> -	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
>>>>> -		&& __copy_from_user(&set.sig[1], &frame->extramask,
>>>>> -				    sizeof(frame->extramask))))
>>>>> +
>>>>> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
>>>>> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
>>>>> +                __get_user(set.sig[0], &frame->sc.oldmask))
>>>>>  		goto badframe;
>>>> I'm not sure why this eliminates that warning.
>>>> set.sig[0] may not be initialized too, if __copy_from_user() failed.
>>> True, but only when either or both of __copy_from_user() and
>>> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
>>> initialized.
>>>
>>>> I don't have enough time to look at this right now, sorry.
>>>>
>>>> Another question, __copy_from_user() will be called even if
>>>> _NSIG_WORDS is less than 2, perhaps it never occurs.
>>>> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
>>>> is better.
>>> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
>>> drop it.
>> If you get the Acked-by from Hiroshi-san it looks good to me. He 
>> modified this code last.
>>
> 
> This seriously looks wrong to me.  If _NSIG_WORDS == 1, then calling
> __copy_from_user here is a serious error.

Right. If _NSIG_WORDS is 1, sigset_t set has only sig[0], writing to
set.sig[1] means stack corruption.

Subrata, could you try like this?
if ((_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1], ...) ||
	__get_user(set.sig[0], ...))

I wonder whether gcc really complains about the case of
__get_user(set.sig[0], ...) failure.
Why, the case which sig[0] initialized and sig[1] uninitialized is NG
and the case which sig[0] uninitialized and sig[1] initialized is OK.

Thanks,
Hiroshi

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Subrata Modak]

On Mon, 2009-05-18 at 12:36 +0900, Hiroshi Shimamoto wrote:
> Subrata Modak wrote:
> > On Fri, 2009-05-15 at 12:32 +0900, Hiroshi Shimamoto wrote:
> >> Subrata Modak wrote:
> >>> Hello Hiroshi-san,
> >>>
> >>> On Thu, 2009-05-14 at 09:24 +0900, Hiroshi Shimamoto wrote:
> >>> H. Peter Anvin wrote:
> >>>>> Ingo Molnar wrote:
> >>>>>>>>>  
> >>>>>>>>>         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
> >>>>>>>>>                 goto badframe;
> >>>>>>>>> -       if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> >>>>>>>>> -               && __copy_from_user(&set.sig[1], &frame->extramask,
> >>>>>>>>> -                                   sizeof(frame->extramask))))
> >>>>>>>>> +
> >>>>>>>>> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
> >>>>>>>>> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
> >>>>>>>>> +                __get_user(set.sig[0], &frame->sc.oldmask))
> >>>>>>>>>                 goto badframe;
> >>>>>>>> I'm not sure why this eliminates that warning.
> >>>>>>>> set.sig[0] may not be initialized too, if __copy_from_user() failed.
> >>>>>>> True, but only when either or both of __copy_from_user() and
> >>>>>>> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
> >>>>>>> initialized.
> >>>>>>>
> >>>>>>>> I don't have enough time to look at this right now, sorry.
> >>>>>>>>
> >>>>>>>> Another question, __copy_from_user() will be called even if
> >>>>>>>> _NSIG_WORDS is less than 2, perhaps it never occurs.
> >>>>>>>> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
> >>>>>>>> is better.
> >>>>>>> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
> >>>>>>> drop it.
> >>>>>> If you get the Acked-by from Hiroshi-san it looks good to me. He 
> >>>>>> modified this code last.
> >>>>>>
> >>>>> This seriously looks wrong to me.  If _NSIG_WORDS == 1, then calling
> >>>>> __copy_from_user here is a serious error.
> >>>> Right. If _NSIG_WORDS is 1, sigset_t set has only sig[0], writing to
> >>>> set.sig[1] means stack corruption.
> >>>>
> >>>> Subrata, could you try like this?
> >>>> if ((_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1], ...) ||
> >>>>       __get_user(set.sig[0], ...))
> >>>>
> >>>>
> >>> How about now ? Thanks for pointing that out. My mistake ;-)
> >> Hi Subrata, I have a question.
> >> Have you tried to compile on x86_64 whether the compiler claims the
> >> similar code in sys32_sigreturn() in arch/x86/ia32/ia32_signal.c?
> > 
> > Oops. No, the compiler does not complain here. It simply compiles fine. 
> > 
> > So, do you want to take a different view for the patch against
> > arch/x86/kernel/signal.c, or, i would resend it with the following
> > things fixed:
> 
> If you don't think this fix is urgent, could you please check whether
> that warning is false positive on 32bit or gcc for 64bit has an issue
> not to complain against ia32 part?
> 

I will try to find out.

> I think arch/x86/kernel/signal.c and arch/x86/ia32/ia32_signal.c should
> have the same code.

Sure. Not a problem. Please drop the patch for the time being.

Regards--
Subrata

> 
> Thanks,
> Hiroshi

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Hiroshi Shimamoto]

Subrata Modak wrote:
> On Fri, 2009-05-15 at 12:32 +0900, Hiroshi Shimamoto wrote:
>> Subrata Modak wrote:
>>> Hello Hiroshi-san,
>>>
>>> On Thu, 2009-05-14 at 09:24 +0900, Hiroshi Shimamoto wrote:
>>> H. Peter Anvin wrote:
>>>>> Ingo Molnar wrote:
>>>>>>>>>  
>>>>>>>>>         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>>>>>>>>                 goto badframe;
>>>>>>>>> -       if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
>>>>>>>>> -               && __copy_from_user(&set.sig[1], &frame->extramask,
>>>>>>>>> -                                   sizeof(frame->extramask))))
>>>>>>>>> +
>>>>>>>>> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
>>>>>>>>> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
>>>>>>>>> +                __get_user(set.sig[0], &frame->sc.oldmask))
>>>>>>>>>                 goto badframe;
>>>>>>>> I'm not sure why this eliminates that warning.
>>>>>>>> set.sig[0] may not be initialized too, if __copy_from_user() failed.
>>>>>>> True, but only when either or both of __copy_from_user() and
>>>>>>> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
>>>>>>> initialized.
>>>>>>>
>>>>>>>> I don't have enough time to look at this right now, sorry.
>>>>>>>>
>>>>>>>> Another question, __copy_from_user() will be called even if
>>>>>>>> _NSIG_WORDS is less than 2, perhaps it never occurs.
>>>>>>>> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
>>>>>>>> is better.
>>>>>>> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
>>>>>>> drop it.
>>>>>> If you get the Acked-by from Hiroshi-san it looks good to me. He 
>>>>>> modified this code last.
>>>>>>
>>>>> This seriously looks wrong to me.  If _NSIG_WORDS == 1, then calling
>>>>> __copy_from_user here is a serious error.
>>>> Right. If _NSIG_WORDS is 1, sigset_t set has only sig[0], writing to
>>>> set.sig[1] means stack corruption.
>>>>
>>>> Subrata, could you try like this?
>>>> if ((_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1], ...) ||
>>>>       __get_user(set.sig[0], ...))
>>>>
>>>>
>>> How about now ? Thanks for pointing that out. My mistake ;-)
>> Hi Subrata, I have a question.
>> Have you tried to compile on x86_64 whether the compiler claims the
>> similar code in sys32_sigreturn() in arch/x86/ia32/ia32_signal.c?
> 
> Oops. No, the compiler does not complain here. It simply compiles fine. 
> 
> So, do you want to take a different view for the patch against
> arch/x86/kernel/signal.c, or, i would resend it with the following
> things fixed:

If you don't think this fix is urgent, could you please check whether
that warning is false positive on 32bit or gcc for 64bit has an issue
not to complain against ia32 part?

I think arch/x86/kernel/signal.c and arch/x86/ia32/ia32_signal.c should
have the same code.

Thanks,
Hiroshi

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Subrata Modak]

On Fri, 2009-05-15 at 12:32 +0900, Hiroshi Shimamoto wrote:
> Subrata Modak wrote:
> > Hello Hiroshi-san,
> > 
> > On Thu, 2009-05-14 at 09:24 +0900, Hiroshi Shimamoto wrote:
> > H. Peter Anvin wrote:
> >>> Ingo Molnar wrote:
> >>>>>>>  
> >>>>>>>         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
> >>>>>>>                 goto badframe;
> >>>>>>> -       if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> >>>>>>> -               && __copy_from_user(&set.sig[1], &frame->extramask,
> >>>>>>> -                                   sizeof(frame->extramask))))
> >>>>>>> +
> >>>>>>> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
> >>>>>>> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
> >>>>>>> +                __get_user(set.sig[0], &frame->sc.oldmask))
> >>>>>>>                 goto badframe;
> >>>>>> I'm not sure why this eliminates that warning.
> >>>>>> set.sig[0] may not be initialized too, if __copy_from_user() failed.
> >>>>> True, but only when either or both of __copy_from_user() and
> >>>>> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
> >>>>> initialized.
> >>>>>
> >>>>>> I don't have enough time to look at this right now, sorry.
> >>>>>>
> >>>>>> Another question, __copy_from_user() will be called even if
> >>>>>> _NSIG_WORDS is less than 2, perhaps it never occurs.
> >>>>>> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
> >>>>>> is better.
> >>>>> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
> >>>>> drop it.
> >>>> If you get the Acked-by from Hiroshi-san it looks good to me. He 
> >>>> modified this code last.
> >>>>
> >>> This seriously looks wrong to me.  If _NSIG_WORDS == 1, then calling
> >>> __copy_from_user here is a serious error.
> >> Right. If _NSIG_WORDS is 1, sigset_t set has only sig[0], writing to
> >> set.sig[1] means stack corruption.
> >>
> >> Subrata, could you try like this?
> >> if ((_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1], ...) ||
> >>       __get_user(set.sig[0], ...))
> >>
> >>
> > 
> > How about now ? Thanks for pointing that out. My mistake ;-)
> 
> Hi Subrata, I have a question.
> Have you tried to compile on x86_64 whether the compiler claims the
> similar code in sys32_sigreturn() in arch/x86/ia32/ia32_signal.c?

Oops. No, the compiler does not complain here. It simply compiles fine. 

So, do you want to take a different view for the patch against
arch/x86/kernel/signal.c, or, i would resend it with the following
things fixed:


> looks good to me.
> BTW who writes the description?
> 
> Acked-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>
> 

Regards--
Subrata

> 
> Thanks,
> Hiroshi

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Hiroshi Shimamoto]

Subrata Modak wrote:
> Hello Hiroshi-san,
> 
> On Thu, 2009-05-14 at 09:24 +0900, Hiroshi Shimamoto wrote:
> H. Peter Anvin wrote:
>>> Ingo Molnar wrote:
>>>>>>>  
>>>>>>>         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>>>>>>                 goto badframe;
>>>>>>> -       if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
>>>>>>> -               && __copy_from_user(&set.sig[1], &frame->extramask,
>>>>>>> -                                   sizeof(frame->extramask))))
>>>>>>> +
>>>>>>> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
>>>>>>> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
>>>>>>> +                __get_user(set.sig[0], &frame->sc.oldmask))
>>>>>>>                 goto badframe;
>>>>>> I'm not sure why this eliminates that warning.
>>>>>> set.sig[0] may not be initialized too, if __copy_from_user() failed.
>>>>> True, but only when either or both of __copy_from_user() and
>>>>> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
>>>>> initialized.
>>>>>
>>>>>> I don't have enough time to look at this right now, sorry.
>>>>>>
>>>>>> Another question, __copy_from_user() will be called even if
>>>>>> _NSIG_WORDS is less than 2, perhaps it never occurs.
>>>>>> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
>>>>>> is better.
>>>>> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
>>>>> drop it.
>>>> If you get the Acked-by from Hiroshi-san it looks good to me. He 
>>>> modified this code last.
>>>>
>>> This seriously looks wrong to me.  If _NSIG_WORDS == 1, then calling
>>> __copy_from_user here is a serious error.
>> Right. If _NSIG_WORDS is 1, sigset_t set has only sig[0], writing to
>> set.sig[1] means stack corruption.
>>
>> Subrata, could you try like this?
>> if ((_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1], ...) ||
>>       __get_user(set.sig[0], ...))
>>
>>
> 
> How about now ? Thanks for pointing that out. My mistake ;-)

Hi Subrata, I have a question.
Have you tried to compile on x86_64 whether the compiler claims the
similar code in sys32_sigreturn() in arch/x86/ia32/ia32_signal.c?

Thanks,
Hiroshi

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Hiroshi Shimamoto]

Subrata Modak wrote:
> Hello Hiroshi-san,
> 
> On Thu, 2009-05-14 at 09:24 +0900, Hiroshi Shimamoto wrote:
> H. Peter Anvin wrote:
>>> Ingo Molnar wrote:
>>>>>>>  
>>>>>>>         if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>>>>>>                 goto badframe;
>>>>>>> -       if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
>>>>>>> -               && __copy_from_user(&set.sig[1], &frame->extramask,
>>>>>>> -                                   sizeof(frame->extramask))))
>>>>>>> +
>>>>>>> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
>>>>>>> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
>>>>>>> +                __get_user(set.sig[0], &frame->sc.oldmask))
>>>>>>>                 goto badframe;
>>>>>> I'm not sure why this eliminates that warning.
>>>>>> set.sig[0] may not be initialized too, if __copy_from_user() failed.
>>>>> True, but only when either or both of __copy_from_user() and
>>>>> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
>>>>> initialized.
>>>>>
>>>>>> I don't have enough time to look at this right now, sorry.
>>>>>>
>>>>>> Another question, __copy_from_user() will be called even if
>>>>>> _NSIG_WORDS is less than 2, perhaps it never occurs.
>>>>>> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
>>>>>> is better.
>>>>> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
>>>>> drop it.
>>>> If you get the Acked-by from Hiroshi-san it looks good to me. He 
>>>> modified this code last.
>>>>
>>> This seriously looks wrong to me.  If _NSIG_WORDS == 1, then calling
>>> __copy_from_user here is a serious error.
>> Right. If _NSIG_WORDS is 1, sigset_t set has only sig[0], writing to
>> set.sig[1] means stack corruption.
>>
>> Subrata, could you try like this?
>> if ((_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1], ...) ||
>>       __get_user(set.sig[0], ...))
>>
>>
> 
> How about now ? Thanks for pointing that out. My mistake ;-)
> 
> Signed-off-by: Subrata Modak <subrata@linux.vnet.ibm.com>
> To: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>,

looks good to me.
BTW who writes the description?

Acked-by: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>

> Cc: H. Peter Anvin <hpa@zytor.com>,
> Cc: Ingo Molnar <mingo@elte.hu>,
> Cc: x86@kernel.org,
> Cc: Sachin P Sant <sachinp@linux.vnet.ibm.com>,
> Cc: Andi Kleen <andi@firstfloor.org>,
> Cc: Andi Kleen <ak@linux.intel.com>,
> Cc: Linux Kernel <linux-kernel@vger.kernel.org>,
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>,
> Cc: Thomas Gleixner <tglx@linutronix.de>,
> Cc: Ingo Molnar <mingo@redhat.com>
> Subject: Re:[PATCH] Fix Warnining in arch/x86/kernel/signal.c
> ---
> 
> --- a/arch/x86/kernel/signal.c	2009-05-14 11:27:15.000000000 +0530
> +++ b/arch/x86/kernel/signal.c	2009-05-14 14:36:24.000000000 +0530
> @@ -576,9 +576,9 @@ unsigned long sys_sigreturn(struct pt_re
>  
>  	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>  		goto badframe;
> -	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> -		&& __copy_from_user(&set.sig[1], &frame->extramask,
> -				    sizeof(frame->extramask))))
> +	if ((_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1],
> +		&frame->extramask, sizeof(frame->extramask))) ||
> +		__get_user(set.sig[0], &frame->sc.oldmask))
>  		goto badframe;
>  
>  	sigdelsetmask(&set, ~_BLOCKABLE);
> 
> ---
> Regards--
> Subrata
> 
>> I wonder whether gcc really complains about the case of
>> __get_user(set.sig[0], ...) failure.
>> Why, the case which sig[0] initialized and sig[1] uninitialized is NG
>> and the case which sig[0] uninitialized and sig[1] initialized is OK.
>>
>> Thanks,
>> Hiroshi
> 

Re: [PATCH] Fix Warnining in arch/x86/kernel/signal.c

[Hiroshi Shimamoto]

Subrata Modak wrote:
> Hello Hiroshi-san,

Hi Subrata,

> 
> On Thu, 2009-05-14 at 09:24 +0900, Hiroshi Shimamoto wrote:
> H. Peter Anvin wrote:
>>> Ingo Molnar wrote:
>>>>>>>  
>>>>>>>  	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>>>>>>>  		goto badframe;
>>>>>>> -	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
>>>>>>> -		&& __copy_from_user(&set.sig[1], &frame->extramask,
>>>>>>> -				    sizeof(frame->extramask))))
>>>>>>> +
>>>>>>> +        if ( (__copy_from_user(&set.sig[1], &frame->extramask,
>>>>>>> +                sizeof(frame->extramask)) && _NSIG_WORDS > 1) || 
>>>>>>> +                __get_user(set.sig[0], &frame->sc.oldmask))
>>>>>>>  		goto badframe;
>>>>>> I'm not sure why this eliminates that warning.
>>>>>> set.sig[0] may not be initialized too, if __copy_from_user() failed.
>>>>> True, but only when either or both of __copy_from_user() and
>>>>> (_NSIG_WORDS > 1) fails. But in all instances set.sig[1] gets
>>>>> initialized.
>>>>>
>>>>>> I don't have enough time to look at this right now, sorry.
>>>>>>
>>>>>> Another question, __copy_from_user() will be called even if
>>>>>> _NSIG_WORDS is less than 2, perhaps it never occurs.
>>>>>> I think, to check _NSIG_WORDS > 1 before calling __copy_from_user()
>>>>>> is better.
>>>>> Fine. Let Ingo/Thomas/Peter decide whether they would like this fix or
>>>>> drop it.
>>>> If you get the Acked-by from Hiroshi-san it looks good to me. He 
>>>> modified this code last.
>>>>
>>> This seriously looks wrong to me.  If _NSIG_WORDS == 1, then calling
>>> __copy_from_user here is a serious error.
>> Right. If _NSIG_WORDS is 1, sigset_t set has only sig[0], writing to
>> set.sig[1] means stack corruption.
>>
>> Subrata, could you try like this?
>> if ((_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1], ...) ||
>> 	__get_user(set.sig[0], ...))
>>
>>
> 
> I tried out and the compiler does not complain in this case.
> Updated Patch below. Please review.

thanks for testing, it looks OK except small nits.
Could you please check with checkpatch.pl?

> 
> Signed-Off-By: Subrata Modak <subrata@linux.vnet.ibm.com>

WARNING: Signed-off-by: is the preferred form

> To: Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>,
> Cc: H. Peter Anvin <hpa@zytor.com>,
> Cc: Ingo Molnar <mingo@elte.hu>,
> Cc: x86@kernel.org,
> Cc: Sachin P Sant <sachinp@linux.vnet.ibm.com>,
> Cc: Andi Kleen <andi@firstfloor.org>,
> Cc: Andi Kleen <ak@linux.intel.com>,
> Cc: Linux Kernel <linux-kernel@vger.kernel.org>,
> Cc: Balbir Singh <balbir@linux.vnet.ibm.com>,
> Cc: Thomas Gleixner <tglx@linutronix.de>,
> Cc: Ingo Molnar <mingo@redhat.com>
> Subject: Re:[PATCH] Fix Warnining in arch/x86/kernel/signal.c
> ---
> 
> --- a/arch/x86/kernel/signal.c	2009-05-14 11:27:15.000000000 +0530
> +++ b/arch/x86/kernel/signal.c	2009-05-14 11:50:52.000000000 +0530
> @@ -576,9 +576,9 @@ unsigned long sys_sigreturn(struct pt_re
>  
>  	if (!access_ok(VERIFY_READ, frame, sizeof(*frame)))
>  		goto badframe;
> -	if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
> -		&& __copy_from_user(&set.sig[1], &frame->extramask,
> -				    sizeof(frame->extramask))))
> +	if ( (_NSIG_WORDS > 1 && __copy_from_user(&set.sig[1],

ERROR: space prohibited after that open parenthesis '('

> +               &frame->extramask, sizeof(frame->extramask))) ||

ERROR: code indent should use tabs where possible

> +               __get_user(set.sig[0], &frame->sc.oldmask))

ERROR: code indent should use tabs where possible

Thanks,
Hiroshi

>  		goto badframe;
>  
>  	sigdelsetmask(&set, ~_BLOCKABLE);
> 
> ---
> Regards--
> Subrata
> 
>> I wonder whether gcc really complains about the case of
>> __get_user(set.sig[0], ...) failure.
>> Why, the case which sig[0] initialized and sig[1] uninitialized is NG
>> and the case which sig[0] uninitialized and sig[1] initialized is OK.
>>
>> Thanks,
>> Hiroshi
>