Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220223231752.52241-1-ppbuk5246@gmail.com> <YhbCGDzlTWp2OJzI@zeniv-ca.linux.org.uk>
 <CAM7-yPTM6FNuT4vs2EuKAKitTWMTHw_XzKVggxQJzn5hqbBHpw@mail.gmail.com> <Yhb0vB/G2a92zJJP@zeniv-ca.linux.org.uk>
In-Reply-To: <Yhb0vB/G2a92zJJP@zeniv-ca.linux.org.uk>
From:   Yun Levi <ppbuk5246@gmail.com>
Date:   Thu, 24 Feb 2022 12:13:37 +0900
Message-ID: <CAM7-yPSorBJzb7NH_Od5mTgDLtgaD-azVNaP0h6-z=aAV_4URQ@mail.gmail.com>
Subject: Re: [PATCH] fs/exec.c: Avoid a race in formats
To:     Al Viro <viro@zeniv.linux.org.uk>
Cc:     Kees Cook <keescook@chromium.org>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        linux-fsdevel@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 24, 2022 at 12:00 PM Al Viro <viro@zeniv.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?