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

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

[Subrata Modak]

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>,
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,
> 
> 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,
> 
> 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

[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:
> 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 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

[Subrata Modak]

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], ...))
> 
> 

I tried out and the compiler does not complain in this case.
Updated Patch below. Please review.

Signed-Off-By: Subrata Modak <subrata@linux.vnet.ibm.com>
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],
+               &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