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

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

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.

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.

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

> 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

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

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.