[PATCH] fs/exec.c: Avoid a race in formats

[PATCH] fs/exec.c: Avoid a race in formats

[Levi Yun]

Suppose a module registers its own binfmt (custom) and formats is like:

+---------+    +----------+    +---------+
| custom  | -> |  format1 | -> | format2 |
+---------+    +----------+    +---------+

and try to call unregister_binfmt with custom NOT in __exit stage.

In that situation, below race scenario can happen.

CPU 0						CPU1
search_binary_handler				...
	read_lock				unregister_binfmt(custom)
	list_for_each_entry			< wait >
	(get custom binfmt)			...
	read_unlock				...
	...					list_del
	custom binfmt return -ENOEXEC
	get next fmt entry (LIST_POISON1)

Because CPU1 set the fmt->lh.next as LIST_POISON1,
CPU 0 get next binfmt as LIST_POISON1.
In that situation, CPU0 try to dereference LIST_POISON1 address and
makes PANIC.

To avoid this situation, check the fmt is valid.
And if it isn't valid, return -EAGAIN.

Signed-off-by: Levi Yun <ppbuk5246@gmail.com>
---
 fs/exec.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/exec.c b/fs/exec.c
index 79f2c9483302..2042a1232656 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1720,6 +1720,12 @@ static int search_binary_handler(struct linux_binprm *bprm)
  retry:
 	read_lock(&binfmt_lock);
 	list_for_each_entry(fmt, &formats, lh) {
+		if (fmt == LIST_POISON1) {
+			read_unlock(&binfmt_lock);
+			retval = -EAGAIN;
+			break;
+		}
+
 		if (!try_module_get(fmt->module))
 			continue;
 		read_unlock(&binfmt_lock);
-- 
2.34.1

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

[Al Viro]

On Thu, Feb 24, 2022 at 08:17:52AM +0900, Levi Yun wrote:
> Suppose a module registers its own binfmt (custom) and formats is like:
> 
> +---------+    +----------+    +---------+
> | custom  | -> |  format1 | -> | format2 |
> +---------+    +----------+    +---------+
> 
> and try to call unregister_binfmt with custom NOT in __exit stage.

Explain, please.  Why would anyone do that?  And how would such
module decide when it's safe to e.g. dismantle data structures
used by methods of that binfmt, etc.?

Could you give more detailed example?  Because it looks like
papering over an inherently unsafe use of binfmt interfaces...

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

[Yun Levi]

On Thu, Feb 24, 2022 at 8:24 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, Feb 24, 2022 at 08:17:52AM +0900, Levi Yun wrote:
> > Suppose a module registers its own binfmt (custom) and formats is like:
> >
> > +---------+    +----------+    +---------+
> > | custom  | -> |  format1 | -> | format2 |
> > +---------+    +----------+    +---------+
> >
> > and try to call unregister_binfmt with custom NOT in __exit stage.
>
> Explain, please.  Why would anyone do that?  And how would such
> module decide when it's safe to e.g. dismantle data structures
> used by methods of that binfmt, etc.?
> Could you give more detailed example?

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.

module can decide it's safe to unload by tracing the stack and
confirming whether some tasks in the custom binfmt's function after it
unregisters its own binfmt.

> Because it looks like papering over an inherently unsafe use of binfmt interfaces..

I think the above example it's quite a trick and stupid.  it's quite
unsafe to use as you mention.
But, misuse allows that situation to happen without any warning.
As a robustness, I just try to avoid above situation But,
I think it's better to restrict unregister binfmt unregister only when
there is no module usage.

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

[Yun Levi]

On Thu, Feb 24, 2022 at 8:59 AM Yun Levi <ppbuk5246@gmail.com> wrote:
>
> On Thu, Feb 24, 2022 at 8:24 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > On Thu, Feb 24, 2022 at 08:17:52AM +0900, Levi Yun wrote:
> > > Suppose a module registers its own binfmt (custom) and formats is like:
> > >
> > > +---------+    +----------+    +---------+
> > > | custom  | -> |  format1 | -> | format2 |
> > > +---------+    +----------+    +---------+
> > >
> > > and try to call unregister_binfmt with custom NOT in __exit stage.
> >
> > Explain, please.  Why would anyone do that?  And how would such
> > module decide when it's safe to e.g. dismantle data structures
> > used by methods of that binfmt, etc.?
> > Could you give more detailed example?
>
> 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.
>
> module can decide it's safe to unload by tracing the stack and
> confirming whether some tasks in the custom binfmt's function after it
> unregisters its own binfmt.
>
> > Because it looks like papering over an inherently unsafe use of binfmt interfaces..
>
> I think the above example it's quite a trick and stupid.  it's quite
> unsafe to use as you mention.
> But, misuse allows that situation to happen without any warning.
> As a robustness, I just try to avoid above situation But,
> I think it's better to restrict unregister binfmt unregister only when
> there is no module usage.

And not only stupid exmaple,
if someone loadable custom binfmt register in __init and __exit via
register and unregister_binfmt,
I think that situation could happen.

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

[Eric W. Biederman]

Yun Levi <ppbuk5246@gmail.com> writes:

> On Thu, Feb 24, 2022 at 8:59 AM Yun Levi <ppbuk5246@gmail.com> wrote:
>>
>> On Thu, Feb 24, 2022 at 8:24 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>> >
>> > On Thu, Feb 24, 2022 at 08:17:52AM +0900, Levi Yun wrote:
>> > > Suppose a module registers its own binfmt (custom) and formats is like:
>> > >
>> > > +---------+    +----------+    +---------+
>> > > | custom  | -> |  format1 | -> | format2 |
>> > > +---------+    +----------+    +---------+
>> > >
>> > > and try to call unregister_binfmt with custom NOT in __exit stage.
>> >
>> > Explain, please.  Why would anyone do that?  And how would such
>> > module decide when it's safe to e.g. dismantle data structures
>> > used by methods of that binfmt, etc.?
>> > Could you give more detailed example?
>>
>> 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.
>>
>> module can decide it's safe to unload by tracing the stack and
>> confirming whether some tasks in the custom binfmt's function after it
>> unregisters its own binfmt.
>>
>> > Because it looks like papering over an inherently unsafe use of binfmt interfaces..
>>
>> I think the above example it's quite a trick and stupid.  it's quite
>> unsafe to use as you mention.
>> But, misuse allows that situation to happen without any warning.
>> As a robustness, I just try to avoid above situation But,
>> I think it's better to restrict unregister binfmt unregister only when
>> there is no module usage.
>
> And not only stupid exmaple,
> if someone loadable custom binfmt register in __init and __exit via
> register and unregister_binfmt,
> I think that situation could happen.

Mostly of what has been happening with binary formats lately is code
removal.

So I humbly suggest the best defense against misuse by modules is to
simply remove "EXPORT_SYMBOL(__register_binfmt)".

Eric

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

[Yun Levi]

> Mostly of what has been happening with binary formats lately is code
> removal.
>
> So I humbly suggest the best defense against misuse by modules is to
> simply remove "EXPORT_SYMBOL(__register_binfmt)".

It could be a solution. but that means the kernel doesn't allow
dynamic binfmt using modules too.
I think the best safe way to remove registered binfmt is ...

unregister binfmt list first ---- (1)
synchronize_rcu_task();
// tasklist stack-check...
unload module.

But for this, there shouldn't happen in the above situation of (1).
If unregister_binfmt has this problem.. I think there is no way to
unload safely for dynamic registered binfmt via module.



On Thu, Feb 24, 2022 at 9:42 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Yun Levi <ppbuk5246@gmail.com> writes:
>
> > On Thu, Feb 24, 2022 at 8:59 AM Yun Levi <ppbuk5246@gmail.com> wrote:
> >>
> >> On Thu, Feb 24, 2022 at 8:24 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >> >
> >> > On Thu, Feb 24, 2022 at 08:17:52AM +0900, Levi Yun wrote:
> >> > > Suppose a module registers its own binfmt (custom) and formats is like:
> >> > >
> >> > > +---------+    +----------+    +---------+
> >> > > | custom  | -> |  format1 | -> | format2 |
> >> > > +---------+    +----------+    +---------+
> >> > >
> >> > > and try to call unregister_binfmt with custom NOT in __exit stage.
> >> >
> >> > Explain, please.  Why would anyone do that?  And how would such
> >> > module decide when it's safe to e.g. dismantle data structures
> >> > used by methods of that binfmt, etc.?
> >> > Could you give more detailed example?
> >>
> >> 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.
> >>
> >> module can decide it's safe to unload by tracing the stack and
> >> confirming whether some tasks in the custom binfmt's function after it
> >> unregisters its own binfmt.
> >>
> >> > Because it looks like papering over an inherently unsafe use of binfmt interfaces..
> >>
> >> I think the above example it's quite a trick and stupid.  it's quite
> >> unsafe to use as you mention.
> >> But, misuse allows that situation to happen without any warning.
> >> As a robustness, I just try to avoid above situation But,
> >> I think it's better to restrict unregister binfmt unregister only when
> >> there is no module usage.
> >
> > And not only stupid exmaple,
> > if someone loadable custom binfmt register in __init and __exit via
> > register and unregister_binfmt,
> > I think that situation could happen.
>
> Mostly of what has been happening with binary formats lately is code
> removal.
>
> So I humbly suggest the best defense against misuse by modules is to
> simply remove "EXPORT_SYMBOL(__register_binfmt)".
>
> Eric

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

[Eric W. Biederman]

Yun Levi <ppbuk5246@gmail.com> writes:

>> Mostly of what has been happening with binary formats lately is code
>> removal.
>>
>> So I humbly suggest the best defense against misuse by modules is to
>> simply remove "EXPORT_SYMBOL(__register_binfmt)".
>
> It could be a solution. but that means the kernel doesn't allow
> dynamic binfmt using modules too.
> I think the best safe way to remove registered binfmt is ...
>
> unregister binfmt list first ---- (1)
> synchronize_rcu_task();
> // tasklist stack-check...
> unload module.
>
> But for this, there shouldn't happen in the above situation of (1).
> If unregister_binfmt has this problem.. I think there is no way to
> unload safely for dynamic registered binfmt via module.

I took a quick look and unregistering in the module exit routine looks
safe, as set_binfmt takes a module reference, and so prevents the module
from being unloaded.

If you can find a bug with existing in-kernel code that would be
interesting.  Otherwise you are making up assumptions that don't current
match the code and saying the code is bugging with respect to
assumptions that do not hold.

The code in the kernel is practical not an implementation of some
abstract that is robust for every possible use case.

Eric

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

[Yun Levi]

On Thu, Feb 24, 2022 at 11:25 AM Eric W. Biederman
<ebiederm@xmission.com> wrote:
> I took a quick look and unregistering in the module exit routine looks
> safe, as set_binfmt takes a module reference, and so prevents the module
> from being unloaded.
>
> If you can find a bug with existing in-kernel code that would be
> interesting.  Otherwise you are making up assumptions that don't current
> match the code and saying the code is bugging with respect to
> assumptions that do not hold.
>
> The code in the kernel is practical not an implementation of some
> abstract that is robust for every possible use case.
>
> Eric

Thanks and sorry for making a noise.

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

[Al Viro]

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.?

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

[Yun Levi]

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?