linux-kernel - Re: [PATCH] fs/exec.c: Avoid a race in formats

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 for Android: free password hash cracker in your pocket

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

Date:   Thu, 24 Feb 2022 12:13:37 +0900
From:   Yun Levi <ppbuk5246@...il.com>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Kees Cook <keescook@...omium.org>,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        linux-fsdevel@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fs/exec.c: Avoid a race in formats

On Thu, Feb 24, 2022 at 12:00 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Thu, Feb 24, 2022 at 08:59:59AM +0900, Yun Levi wrote:
>
> > I think if someone wants to control their own binfmt via "ioctl" not
> > on time on LOAD.
> > For example, someone wants to control exec (notification,
> > allow/disallow and etc..)
> > and want to enable and disable own's control exec via binfmt reg / unreg
> > In that situation, While the module is loaded, binfmt is still live
> > and can be reused by
> > reg/unreg to enable/disable his exec' control.
>
> Er...  So have your ->load_binary() start with
>         if (I_want_it_disabled)
>                 return -ENOEXEC;
> and be done with that.
> The only caller of that thing is
>         list_for_each_entry(fmt, &formats, lh) {
>                 if (!try_module_get(fmt->module))
>                         continue;
>                 read_unlock(&binfmt_lock);
>
>                 retval = fmt->load_binary(bprm);
>
>                 read_lock(&binfmt_lock);
>                 put_binfmt(fmt);
>                 if (bprm->point_of_no_return || (retval != -ENOEXEC)) {
>                         read_unlock(&binfmt_lock);
>                         return retval;
>                 }
>         }
> so returning -ENOEXEC is equivalent to not having it in the list.
> IDGI...  Why bother unregistering/re-registering/etc.?

For example, someone wants to control exec via policy (allow or deny exec)
In that case, it just wants to confirm the policy NOT LOAD binary but
want to pass
the LOAD to the next binfmt (That's the reason __register_binfmt with insert).
So, To do this, register linux_binfmt with its own with load_binary
function like

    if (this binary allow to run)
                return -ENOEXEC; // pass to next binfmt to load that binary
    else if (deny)
               return -1;

And enable / disable is determined by registered or unregistered status.
That mean

// ioctl hook for enable
         // enable by register binfmt
         __register_binfmt(binfmt, 1);

// ioctl hook for disable
         // disable by unreigster binfmt
        __unregister_binfmt(binfmt);

Because, __unregister_binfmt isn't called int module __exit, but call
while the module is live,
it makes a problem.

It looks so strange, But in the case of the kernel without FTRACE,
BPF, KPROBE, etc
I think there's no other way to  control exec running.
So I just do stupid test :)

But When I read Eric's answer,
I think __unregister_binfmt should be only called in the module __exit
function...
right?