From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760215AbYEVEgs (ORCPT ); Thu, 22 May 2008 00:36:48 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751062AbYEVEgg (ORCPT ); Thu, 22 May 2008 00:36:36 -0400 Received: from cn.fujitsu.com ([222.73.24.84]:58060 "EHLO song.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1750820AbYEVEgf (ORCPT ); Thu, 22 May 2008 00:36:35 -0400 Message-ID: <4834F7F3.8050800@cn.fujitsu.com> Date: Thu, 22 May 2008 12:34:59 +0800 From: Li Zefan User-Agent: Thunderbird 2.0.0.9 (X11/20071115) MIME-Version: 1.0 To: Andrew Morton CC: Shi Weihua , ltp-list@lists.sourceforge.net, LKML , jmorris@namei.org, linux-security-module@vger.kernel.org, morgan@kernel.org Subject: Re: [LTP] [PATCH] fix sys_prctl() returned uninitialized value References: <4834E639.2010209@cn.fujitsu.com> <20080521203212.ddf05254.akpm@linux-foundation.org> In-Reply-To: <20080521203212.ddf05254.akpm@linux-foundation.org> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Andrew Morton wrote: > On Thu, 22 May 2008 11:19:21 +0800 Shi Weihua wrote: > >> When we test kernel by the latest LTP(20080430) on ia64, >> the following failure occured: >> ------------------------------------- >> prctl01 1 PASS : Test Passed >> prctl01 0 WARN : prctl() returned 2048 errno = 0 : Success >> prctl01 1 PASS : Test Passed >> prctl01 2 FAIL : Test Failed >> ------------------------------------- >> >> We found commit 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c >> causes this failure by git-bisect. >> And, we found 'error' has not been initialized in the function >> sys_prctl()(kernel/sys.c). When the capability module is not taking >> responsibility for this call, sys_prctl() may return a wrong value. >> >> Now, i fixed it. >> >> Signed-off-by: Shi Weihua >> --- >> diff --git a/kernel/sys.c b/kernel/sys.c >> index 895d2d4..26eb0f7 100644 >> --- a/kernel/sys.c >> +++ b/kernel/sys.c >> @@ -1652,7 +1652,7 @@ asmlinkage long sys_umask(int mask) >> asmlinkage long sys_prctl(int option, unsigned long arg2, unsigned long arg3, >> unsigned long arg4, unsigned long arg5) >> { >> - long uninitialized_var(error); >> + long error = 0; >> >> if (security_task_prctl(option, arg2, arg3, arg4, arg5, &error)) >> return error; > > Oh dear, there are so many things wrong with this... > > - if security_task_prctl() is returning "fail" then why on earth > isn't it setting the error code? > See comments in security.h: * @task_prctl: ... * @rc_p contains a pointer to communicate back the forced return code * Return 0 if permission is granted, and non-zero if the security module * has taken responsibility (setting *rc_p) for the prctl call. But I don't know why can't just set *rc_p to 0 before returning 0 (as Shi's previous patch did). > - cap_task_prctl() _does_ set `error' is if returns non-zero, so it > must be one of the other myriad backend implementations of > security_task_prctl() which is busted. Which one is it? > > - With the above patch applied, sys_prctl() will return zero (ie: > "success") even though it just failed. > This won't happen. We initialize error to 0, and it will be set to some error value when it should be. The alternative is to set error to -EFXXX or 0 in every switch cases. > - Can't we remove the sixth argument to security_task_prctl() and > just return the result code like a sane function would do? > It used to have 5 arguments, but this commit changed it (and caused this ltp failure): 3898b1b4ebff8dcfbcf1807e0661585e06c9a91c (capabilities: implement per-process securebits)