linux-kernel - Re: [PATCH] kprobes: Rollback kprobe flags on failed arm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CACueBy42VUp=4-nMRfAERmLxEwXkc0tGqWQk935oqLeJ51OU5A@mail.gmail.com>
Date:   Sat, 4 Jun 2022 10:16:52 +0800
From:   chuang <nashuiliang@...il.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     Jingren Zhou <zhoujingren@...iglobal.com>,
        "Naveen N. Rao" <naveen.n.rao@...ux.ibm.com>,
        Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kprobes: Rollback kprobe flags on failed arm_kprobe

Thanks for your quick reply.
I'm very, very sorry for sending multiple emails. I am submitting a
patch for the first time.

On Fri, Jun 3, 2022 at 11:03 PM Masami Hiramatsu <mhiramat@...nel.org> wrote:
>
> This should go to stable, so add below tag. (No need to CC to stable)
>
> Fixes: 12310e343755 ("kprobes: Propagate error from arm_kprobe_ftrace()")
> Cc: stable@...r.kernel.org

Thanks for your kind reminder.

>
> And could you also update this patch as below?
>
> > ---
> >  kernel/kprobes.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index f214f8c088ed..96c75e23113c 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2422,8 +2422,10 @@ int enable_kprobe(struct kprobe *kp)
> >       if (!kprobes_all_disarmed && kprobe_disabled(p)) {
> >               p->flags &= ~KPROBE_FLAG_DISABLED;
> >               ret = arm_kprobe(p);
> > -             if (ret)
> > +             if (ret) {
> >                       p->flags |= KPROBE_FLAG_DISABLED;
>
> Here, can you add a check?
>
>         if (p != kp)
>
> > +                     kp->flags |= KPROBE_FLAG_DISABLED;

Well, I also thought about it. This already covers conditions `p ==
kp` and `p != kp`:
---
                       kp->flags |= KPROBE_FLAG_DISABLED;
---

When p is equal to kp, `kp->flags` is assigned to KPROBE_FLAG_DISABLED twice.
However, if you add a check that p is not equal to kp, this makes the
code read more clearly. Anything is OK, what is your suggestion?

>
> Thus is is clear that this is corresponding to
> ---
>         if (p != kp)
>                 kp->flags &= ~KPROBE_FLAG_DISABLED;
> ---
>
> Thank you,
>
> > +             }
> >       }
> >  out:
> >       mutex_unlock(&kprobe_mutex);
> > --
> > 2.34.1
> >
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@...nel.org>

Have a wonderful day!