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 <[email protected]> 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 <[email protected]> wrote:
>
> On Thu, Feb 24, 2022 at 8:24 AM Al Viro <[email protected]> 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 <[email protected]> writes:
> On Thu, Feb 24, 2022 at 8:59 AM Yun Levi <[email protected]> wrote:
>>
>> On Thu, Feb 24, 2022 at 8:24 AM Al Viro <[email protected]> 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 <[email protected]> wrote:
>
> Yun Levi <[email protected]> writes:
>
> > On Thu, Feb 24, 2022 at 8:59 AM Yun Levi <[email protected]> wrote:
> >>
> >> On Thu, Feb 24, 2022 at 8:24 AM Al Viro <[email protected]> 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 <[email protected]> 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
<[email protected]> 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.
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.?
On Thu, Feb 24, 2022 at 12:00 PM Al Viro <[email protected]> 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?