[PATCH] CRED: Fixup credentials build breakage - Kernel

This is a discussion on [PATCH] CRED: Fixup credentials build breakage - Kernel ; A recent patch titled: CRED: Separate task security context from task_struct removed task security context from task_struct, but did not update all locations to use the new struct cred that was introduced. The change to task_struct broke perfmon and ia32 ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: [PATCH] CRED: Fixup credentials build breakage

  1. [PATCH] CRED: Fixup credentials build breakage

    A recent patch titled:

    CRED: Separate task security context from task_struct

    removed task security context from task_struct, but did not
    update all locations to use the new struct cred that was
    introduced.

    The change to task_struct broke perfmon and ia32 syscalls on
    ia64. This patch fixes the build.
    ---
    All things considered, I'd prefer to see this patch folded into
    7931c65268777ed10cab22486de149d742a1f269 so we can keep
    bisectability. Would that be possible, given that these changes
    are "only" in linux-next and haven't hit Linus's tree yet?

    My hope for folding in this patch into the bigger patch is the
    reason why I didn't refer to the commit by SHA1 in the
    changelog...

    Also, I'm not exactly sure why wrappers were provided for
    task_gid, but then later removed. Additionally, I wasn't sure why
    no wrapper was provided for task_suid and friends. But I admit
    that I didn't read the patch series in depth; only enough to fix
    the build.

    arch/ia64/ia32/sys_ia32.c | 8 ++++----
    arch/ia64/kernel/perfmon.c | 24 ++++++++++++------------
    2 files changed, 16 insertions(+), 16 deletions(-)

    diff --git a/arch/ia64/ia32/sys_ia32.c b/arch/ia64/ia32/sys_ia32.c
    index 465116a..950b63a 100644
    --- a/arch/ia64/ia32/sys_ia32.c
    +++ b/arch/ia64/ia32/sys_ia32.c
    @@ -2089,20 +2089,20 @@ sys32_getgroups16 (int gidsetsize, short __user *grouplist)
    if (gidsetsize < 0)
    return -EINVAL;

    - get_group_info(current->group_info);
    - i = current->group_info->ngroups;
    + get_group_info(current->cred->group_info);
    + i = current->cred->group_info->ngroups;
    if (gidsetsize) {
    if (i > gidsetsize) {
    i = -EINVAL;
    goto out;
    }
    - if (groups16_to_user(grouplist, current->group_info)) {
    + if (groups16_to_user(grouplist, current->cred->group_info)) {
    i = -EFAULT;
    goto out;
    }
    }
    out:
    - put_group_info(current->group_info);
    + put_group_info(current->cred->group_info);
    return i;
    }

    diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
    index ffe6de0..7df49bc 100644
    --- a/arch/ia64/kernel/perfmon.c
    +++ b/arch/ia64/kernel/perfmon.c
    @@ -2410,18 +2410,18 @@ pfm_bad_permissions(struct task_struct *task)
    DPRINT(("cur: uid=%d gid=%d task: euid=%d suid=%d uid=%d egid=%d sgid=%d\n",
    uid,
    gid,
    - task->euid,
    - task->suid,
    - task->uid,
    - task->egid,
    - task->sgid));
    -
    - return (uid != task->euid)
    - || (uid != task->suid)
    - || (uid != task->uid)
    - || (gid != task->egid)
    - || (gid != task->sgid)
    - || (gid != task->gid)) && !capable(CAP_SYS_PTRACE);
    + task_euid(task),
    + task->cred->suid,
    + task_uid(task),
    + task->cred->egid,
    + task->cred->sgid));
    +
    + return ((uid != task_euid(task))
    + || (uid != task->cred->suid)
    + || (uid != task_uid(task))
    + || (gid != task->cred->egid)
    + || (gid != task->cred->sgid)
    + || (gid != task->cred->gid)) && !capable(CAP_SYS_PTRACE);
    }

    static int
    --
    1.5.3.1.gbed62

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] CRED: Fixup credentials build breakage

    Alex Chiang wrote:

    > A recent patch titled:
    >
    > CRED: Separate task security context from task_struct
    >
    > removed task security context from task_struct, but did not
    > update all locations to use the new struct cred that was
    > introduced.
    >
    > The change to task_struct broke perfmon and ia32 syscalls on
    > ia64. This patch fixes the build.


    I've submitted a patch to fix this.

    > All things considered, I'd prefer to see this patch folded into
    > 7931c65268777ed10cab22486de149d742a1f269 so we can keep
    > bisectability. Would that be possible, given that these changes
    > are "only" in linux-next and haven't hit Linus's tree yet?


    Probably. I'll have to talk to Stephen about how to do this. I can't maintain
    my patches on top of linux-next now:-/

    > Also, I'm not exactly sure why wrappers were provided for
    > task_gid, but then later removed. Additionally, I wasn't sure why
    > no wrapper was provided for task_suid and friends. But I admit
    > that I didn't read the patch series in depth; only enough to fix
    > the build.


    Generally, accesses to task->gid are accompanied by accesses to task->uid
    and/or task->groups, in which case using multiple wrappers in series introduces
    excess locking and superfluous memory barriers. To access task->gid, one must
    take the RCU read lock and then use rcu_dereference().

    However, I can't use the cred struct until the patch in which it is introduced.
    The wrappers, however, allow me to grep for things I've missed, and thus make
    it easier to do things in stages.

    Your patch also has a couple of issues:

    > + get_group_info(current->cred->group_info);


    The call to get_group_info() is not necessary. Only current may change its own
    groups.

    > + || (uid != task->cred->suid)
    > + || (gid != task->cred->egid)
    > + || (gid != task->cred->sgid)
    > + || (gid != task->cred->gid))


    This is incorrect. You must hold the RCU read lock whilst you make this
    access, and you must pass task->cred through rcu_dereference(), or better
    still, use __task_cred(task) to get at it.

    My patch is below. Note that there is a problem with it, presumably something
    to do with GIT's merging (the ad1848 files being deleted).

    David
    ---
    Fix the IA64 arch's use of COW credentials.

    Signed-off-by: David Howells
    ---

    arch/ia64/ia32/sys_ia32.c | 7 +++----
    arch/ia64/kernel/perfmon.c | 32 ++++++++++++++++++++------------
    include/sound/ad1848.h | 0
    sound/isa/ad1848/ad1848_lib.c | 0
    4 files changed, 23 insertions(+), 16 deletions(-)
    delete mode 100644 include/sound/ad1848.h
    delete mode 100644 sound/isa/ad1848/ad1848_lib.c

    diff --git a/arch/ia64/ia32/sys_ia32.c b/arch/ia64/ia32/sys_ia32.c
    index 465116a..7f0704f 100644
    --- a/arch/ia64/ia32/sys_ia32.c
    +++ b/arch/ia64/ia32/sys_ia32.c
    @@ -2084,25 +2084,24 @@ groups16_from_user(struct group_info *group_info, short __user *grouplist)
    asmlinkage long
    sys32_getgroups16 (int gidsetsize, short __user *grouplist)
    {
    + const struct cred *cred = current_cred();
    int i;

    if (gidsetsize < 0)
    return -EINVAL;

    - get_group_info(current->group_info);
    - i = current->group_info->ngroups;
    + i = cred->group_info->ngroups;
    if (gidsetsize) {
    if (i > gidsetsize) {
    i = -EINVAL;
    goto out;
    }
    - if (groups16_to_user(grouplist, current->group_info)) {
    + if (groups16_to_user(grouplist, cred->group_info)) {
    i = -EFAULT;
    goto out;
    }
    }
    out:
    - put_group_info(current->group_info);
    return i;
    }

    diff --git a/arch/ia64/kernel/perfmon.c b/arch/ia64/kernel/perfmon.c
    index ffe6de0..a1aead7 100644
    --- a/arch/ia64/kernel/perfmon.c
    +++ b/arch/ia64/kernel/perfmon.c
    @@ -2403,25 +2403,33 @@ error_kmem:
    static int
    pfm_bad_permissions(struct task_struct *task)
    {
    + const struct cred *tcred;
    uid_t uid = current_uid();
    gid_t gid = current_gid();
    + int ret;
    +
    + rcu_read_lock();
    + tcred = __task_cred(task);

    /* inspired by ptrace_attach() */
    DPRINT(("cur: uid=%d gid=%d task: euid=%d suid=%d uid=%d egid=%d sgid=%d\n",
    uid,
    gid,
    - task->euid,
    - task->suid,
    - task->uid,
    - task->egid,
    - task->sgid));
    -
    - return (uid != task->euid)
    - || (uid != task->suid)
    - || (uid != task->uid)
    - || (gid != task->egid)
    - || (gid != task->sgid)
    - || (gid != task->gid)) && !capable(CAP_SYS_PTRACE);
    + tcred->euid,
    + tcred->suid,
    + tcred->uid,
    + tcred->egid,
    + tcred->sgid));
    +
    + ret = ((uid != tcred->euid)
    + || (uid != tcred->suid)
    + || (uid != tcred->uid)
    + || (gid != tcred->egid)
    + || (gid != tcred->sgid)
    + || (gid != tcred->gid)) && !capable(CAP_SYS_PTRACE);
    +
    + rcu_read_unlock();
    + return ret;
    }

    static int
    diff --git a/include/sound/ad1848.h b/include/sound/ad1848.h
    deleted file mode 100644
    index e69de29..0000000
    diff --git a/sound/isa/ad1848/ad1848_lib.c b/sound/isa/ad1848/ad1848_lib.c
    deleted file mode 100644
    index e69de29..0000000

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] CRED: Fixup credentials build breakage

    * David Howells :
    > Alex Chiang wrote:
    >
    > > A recent patch titled:
    > >
    > > CRED: Separate task security context from task_struct
    > >
    > > removed task security context from task_struct, but did not
    > > update all locations to use the new struct cred that was
    > > introduced.
    > >
    > > The change to task_struct broke perfmon and ia32 syscalls on
    > > ia64. This patch fixes the build.

    >
    > I've submitted a patch to fix this.


    Thanks, clearly your patch is correct and mine was not. I was
    just trying to get my build going again. I can apply yours by
    hand to my own tree here to get going.

    > > All things considered, I'd prefer to see this patch folded
    > > into 7931c65268777ed10cab22486de149d742a1f269 so we can keep
    > > bisectability. Would that be possible, given that these
    > > changes are "only" in linux-next and haven't hit Linus's tree
    > > yet?

    >
    > Probably. I'll have to talk to Stephen about how to do this.
    > I can't maintain my patches on top of linux-next now:-/


    Ok, I still think this would be a good idea, so if it's possible,
    that would be great.

    Thanks,

    /ac

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread