linux-kernel - Re: [PATCH v2] tracing: fix bad use of igrab in trace

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] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 20 Apr 2018 16:34:25 +0200
From:   Miklos Szeredi <miklos@...redi.hu>
To:     Song Liu <songliubraving@...com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Howard McLauchlan <hmclauchlan@...com>,
        Josef Bacik <jbacik@...com>,
        Srikar Dronamraju <srikar@...ux.vnet.ibm.com>
Subject: Re: [PATCH v2] tracing: fix bad use of igrab in trace_uprobe.c

On Thu, Apr 19, 2018 at 6:37 PM, Song Liu <songliubraving@...com> wrote:
>
>
>> On Apr 19, 2018, at 7:44 AM, Miklos Szeredi <miklos@...redi.hu> wrote:
>>
>> On Thu, Apr 19, 2018 at 10:58 AM, Miklos Szeredi <miklos@...redi.hu> wrote:
>>> On Wed, Apr 18, 2018 at 7:40 PM, Song Liu <songliubraving@...com> wrote:

>>>>        *arg++ = '\0';
>>>>        filename = argv[1];
>>>>        ret = kern_path(filename, LOOKUP_FOLLOW, &path);
>>>>        if (ret)
>>>> -               goto fail_address_parse;
>>>> -
>>>> -       inode = igrab(d_real_inode(path.dentry));
>>
>> Also, where has the d_real_inode() gone?
>>
>> Looks like we need tu->inode back, since the return value of
>> d_real_inode() may change over time.   I'd do the "tu->inode =
>> d_real_inode(tu->path.dentry)" just before first use (i.e. when
>> enabling the tracepoint).
>>

>
> Do we need mechanism to prevent the return value of d_real_inode()
> to change? Would the following sequence happen?
>
>     create trace_uprobe
>     enable trace_uprobe (uprobe_register)
>     d_real changes
>     disable trace_uprobe (uprobe_unregister get wrong inode?)

Yes.

>
> Another case might be:
>
>     create trace_uprobe
>     enable trace_uprobe (uprobe_register)
>     disable trace_uprobe (uprobe_unregister)
>     d_real changes
>     enable trace_uprobe (do we need new inode for uprobe_register)

Probably a good idea to use the new one, but doesn't really matter.
Do the one that's simpler.

This corner case is simply not interesting (modifying a binary while
it is being debugged with uprobe).  Let's just concentrate on making
this crash and leak free.

Thanks,
Miklos