lkml.org 
[lkml]   [2008]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [LTP] [PATCH] fix sys_prctl() returned uninitialized value
    On Thu, 22 May 2008 12:34:59 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:

    > Andrew Morton wrote:
    > > On Thu, 22 May 2008 11:19:21 +0800 Shi Weihua <shiwh@cn.fujitsu.com> 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 <shiwh@cn.fujitsu.com>
    > >> ---
    > >> 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 that didn't happen! As you've discovered, security_task_prctl()
    returns non-zero but _doesn't_ set *rc_p.

    Which, as I said, is inconsistent with cap_task_prctl(). As well as
    being downright peculiar.

    > But I don't know why can't just set *rc_p to 0 before returning 0 (as Shi's previous
    > patch did).

    Zeroing it seems wrong. It should return the _reason_ for the non-zero
    return?

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

    If that were true, we'd never be returning an uninitialised value.

    Unless there's some code somewhere which is doing

    if (*rc_p != 0)
    *rc_p = something;

    which there might be. In which case the entire patch makes complete
    sense, but a) it needs changelog repair and b) I'd suggest that the
    zeroing should be done in security_task_prctl() instead and c) someone
    needs to work out what the actual interface is to this thing and document
    it properly.

    If that interface is deemed to be "randomly returns inexplicable garbage
    in `error'" then fine, perhaps sys_prctl() should just return literal zero
    in this case and ignore `error'. Even though returning zero in that case
    seems exactly wrong.

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

    hm, I suppose there was a reason.


    Sorry, but the whole thing looks screwed up to me.


    \
     
     \ /
      Last update: 2008-05-22 07:25    [W:0.035 / U:0.220 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site