lkml.org 
[lkml]   [2010]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] cred - BUG: synchronize rcu before releasing cred
    hi,
    any feedback?

    jirka

    On Fri, Jul 02, 2010 at 02:14:56PM +0200, Jiri Olsa wrote:
    > hi,
    > any feedback?
    >
    > jirka
    >
    > On Fri, Jun 25, 2010 at 09:33:33AM -0400, Jiri Olsa wrote:
    > > BZ 591015 - kernel BUG at kernel/cred.c:168
    > > https://bugzilla.redhat.com/show_bug.cgi?id=591015
    > >
    > > Above bugzilla reported bug during the releasing of
    > > old cred structure.
    > >
    > > There is reproducer attached to the bugzilla.
    > >
    > > The issue is caused by releasing old cred struct while other
    > > kernel path might be still using it. This leads to cred->usage
    > > inconsistency inside the __put_cred and triggering the bug.
    > >
    > > Following kernel paths are affected:
    > >
    > > The CPU1 path is setting the new groups creds.
    > > The CPU2 path is cat /proc/PID/status
    > >
    > >
    > > CPU 1 CPU 2
    > >
    > > sys_setgroups proc_pid_status
    > > set_current_groups task_state
    > > commit_creds rcu_read_lock
    > > put_cred ...
    > > __put_cred get_cred
    > > BUG_ON(usage != 0) ...
    > > rcu_read_unlock
    > >
    > >
    > >
    > > If __put_cred got executed during the CPU2 holding the reference
    > > the BUG_ON inside __put_cred is trigered.
    > >
    > > I think there's no need to get the cred refference as long as
    > > the 'cred' handling stays inside the rcu_read_lock block.
    > >
    > > And the condition of __task_cred 'make sure task doesn't go away',
    > > is done by proc_single_show as this is the proc file.
    > >
    > > wbr,
    > > jirka
    > >
    > >
    > > Signed-off-by: Jiri Olsa <jolsa@redhat.com>
    > > Acked-by: David Howells <dhowells@redhat.com>
    > > ---
    > > diff --git a/fs/proc/array.c b/fs/proc/array.c
    > > index 9b58d38..ac3b3a4 100644
    > > --- a/fs/proc/array.c
    > > +++ b/fs/proc/array.c
    > > @@ -176,7 +176,7 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
    > > if (tracer)
    > > tpid = task_pid_nr_ns(tracer, ns);
    > > }
    > > - cred = get_cred((struct cred *) __task_cred(p));
    > > + cred = __task_cred(p);
    > > seq_printf(m,
    > > "State:\t%s\n"
    > > "Tgid:\t%d\n"
    > > @@ -199,15 +199,14 @@ static inline void task_state(struct seq_file *m, struct pid_namespace *ns,
    > > "FDSize:\t%d\n"
    > > "Groups:\t",
    > > fdt ? fdt->max_fds : 0);
    > > - rcu_read_unlock();
    > >
    > > group_info = cred->group_info;
    > > task_unlock(p);
    > >
    > > for (g = 0; g < min(group_info->ngroups, NGROUPS_SMALL); g++)
    > > seq_printf(m, "%d ", GROUP_AT(group_info, g));
    > > - put_cred(cred);
    > >
    > > + rcu_read_unlock();
    > > seq_printf(m, "\n");
    > > }
    > >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html


    \
     
     \ /
      Last update: 2010-07-27 14:17    [W:0.029 / U:119.488 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site