lkml.org 
[lkml]   [2011]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [kernel-hardening] [PATCH 2/2] security: Yama LSM
    From
    On Tue, Dec 20, 2011 at 9:25 PM, John Johansen
    <john.johansen@canonical.com> wrote:
    > On 12/19/2011 02:17 PM, Kees Cook wrote:
    >> This adds the Yama Linux Security Module to collect DAC security
    >> improvements (specifically just ptrace restrictions for now) that have
    >> existed in various forms over the years and have been carried outside the
    >> mainline kernel by other Linux distributions like Openwall and grsecurity.
    >>
    >> Signed-off-by: Kees Cook <keescook@chromium.org>
    >
    > This is looking really good.  I have a few notes inlined below but I didn't
    > find any problems.
    >
    > I am happy giving it an
    > Acked-by: John Johansen <john.johansen@canonical.com>

    Thanks for the review!

    >> +/**
    >> + * yama_ptracer_add - add/replace an exception for this tracer/tracee pair
    >> + * @tracer: the task_struct of the process doing the ptrace
    >> + * @tracee: the task_struct of the process to be ptraced
    >> + *
    >> + * Returns 0 if relationship was added, -ve on error.
    >> + */
    > It might be good to add a note here, or a more explicit note in the Yama
    > Documentation that a tracee can only have a single tracer (+ its descendants).

    Good idea; I've added a note in both places now.

    >> +static int yama_ptracer_add(struct task_struct *tracer,
    >> +                         struct task_struct *tracee)
    >> +{
    >> +     int rc = 0;
    >> +     struct ptrace_relation *added;
    >> +     struct ptrace_relation *entry, *relation = NULL;
    >> +
    >> +     added = kmalloc(sizeof(*added), GFP_KERNEL);
    >> +     spin_lock_bh(&ptracer_relations_lock);
    >> +     list_for_each_entry(entry, &ptracer_relations, node)
    >> +             if (entry->tracee == tracee) {
    >> +                     relation = entry;
    >> +                     break;
    >> +             }
    >> +     if (!relation) {
    >> +             relation = added;
    >> +             if (!relation) {
    >> +                     rc = -ENOMEM;
    >> +                     goto unlock_out;
    >> +             }
    >> +             relation->tracee = tracee;
    >> +             list_add(&relation->node, &ptracer_relations);
    >> +     }
    >> +     relation->tracer = tracer;
    >> +
    >> +unlock_out:
    >> +     spin_unlock_bh(&ptracer_relations_lock);
    >> +     if (added && added != relation)
    >> +             kfree(added);
    >> +
    >> +     return rc;
    >> +}
    >> +
    > I think pulling the out of the locking makes the fn a little easier to
    > read.

    Agreed, I've done this now. I think my original thought was "why check
    'added' unless we actually need it?" But it just makes the whole thing
    harder to read.

    >> +     spin_lock_bh(&ptracer_relations_lock);
    >> +     list_for_each_safe(list, safe, &ptracer_relations) {
    >> +             relation = list_entry(list, struct ptrace_relation, node);
    > You could use list_for_each_entry_safe here
    >        list_for_each_entry_safe(relation, safe, &ptracer_relations, node)

    Ah! Yes, thanks for catching this.

    >> + * yama_task_prctl - check for Yama-specific prctl operations
    >> + * @option: operation
    >> + * @arg2: argument
    >> + * @arg3: argument
    >> + * @arg4: argument
    >> + * @arg5: argument
    >> + *
    >> + * Return 0 on success, -ve on error.  -ENOSYS is returned when Yama
    >> + * does not handle the given option.
    >> + */
    > So maybe add a note here about why the tracee is being stored via the
    > group_leader, but that the tracer isn't.  Its not really a problem but I
    > tripped over this at first as I was going through the code.

    Yeah, dealing with when a thread might be calling these things lead to
    some confusion. In the end, I have to check group_leader in a few
    places, and this was a seemingly good place too. I might rework this a
    bit, but in the meantime, I've added a comment explaining my
    reasoning.

    Thanks!

    -Kees

    --
    Kees Cook
    ChromeOS Security
    --
    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/

    \
     
     \ /
      Last update: 2011-12-21 21:21    [W:0.247 / U:60.700 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site