While syzkaller is finding many bugs, sometimes syzkaller examines
stupid operations. Currently we prevent syzkaller from examining
stupid operations by blacklisting syscall arguments and/or disabling
whole functionality using existing kernel config options, but it is
a whack-a-mole approach. We need cooperation from kernel side [1].
This patch introduces a kernel config option which allows disabling
only specific operations. This kernel config option should be enabled
only when building kernels for fuzz testing.
We discussed possibility of disabling specific operations at run-time
using some lockdown mechanism [2], but conclusion seems that build-time
control (i.e. kernel config option) fits better for this purpose.
Since patches for users of this kernel config option will want a lot of
explanation [3], this patch provides only kernel config option for them.
[1] https://github.com/google/syzkaller/issues/1622
[2] https://lkml.kernel.org/r/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com
[3] https://lkml.kernel.org/r/[email protected]
Signed-off-by: Tetsuo Handa <[email protected]>
Cc: Dmitry Vyukov <[email protected]>
---
lib/Kconfig.debug | 10 ++++++++++
1 file changed, 10 insertions(+)
Changes since v1:
Drop users of this kernel config option.
Update patch description.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 53e786e0a604..e360090e24c5 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2208,4 +2208,14 @@ config HYPERV_TESTING
endmenu # "Kernel Testing and Coverage"
+config KERNEL_BUILT_FOR_FUZZ_TESTING
+ bool "Build kernel for fuzz testing"
+ default n
+ help
+ Say N unless you are building kernels for fuzz testing.
+ Saying Y here disables several things that legitimately cause
+ damage under a fuzzer workload (e.g. copying to arbitrary
+ user-specified kernel address, changing console loglevel,
+ freezing filesystems).
+
endmenu # Kernel hacking
--
2.18.2
On 07. 03. 20, 14:58, Tetsuo Handa wrote:
> While syzkaller is finding many bugs, sometimes syzkaller examines
> stupid operations. Currently we prevent syzkaller from examining
> stupid operations by blacklisting syscall arguments and/or disabling
> whole functionality using existing kernel config options, but it is
> a whack-a-mole approach. We need cooperation from kernel side [1].
>
> This patch introduces a kernel config option which allows disabling
> only specific operations. This kernel config option should be enabled
> only when building kernels for fuzz testing.
>
> We discussed possibility of disabling specific operations at run-time
> using some lockdown mechanism [2], but conclusion seems that build-time
> control (i.e. kernel config option) fits better for this purpose.
> Since patches for users of this kernel config option will want a lot of
> explanation [3], this patch provides only kernel config option for them.
>
> [1] https://github.com/google/syzkaller/issues/1622
> [2] https://lkml.kernel.org/r/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com
> [3] https://lkml.kernel.org/r/[email protected]
>
> Signed-off-by: Tetsuo Handa <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> ---
> lib/Kconfig.debug | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> Changes since v1:
> Drop users of this kernel config option.
> Update patch description.
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 53e786e0a604..e360090e24c5 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2208,4 +2208,14 @@ config HYPERV_TESTING
>
> endmenu # "Kernel Testing and Coverage"
>
> +config KERNEL_BUILT_FOR_FUZZ_TESTING
> + bool "Build kernel for fuzz testing"
If we really want to go this way, I wouldn't limit it for fuzz testing
only. Static analyzers, symbolic executors, formal verifiers, etc. --
all of them should avoid the paths.
So what about KERNEL_BUILT_FOR_ANALYZERS?
> + default n
> + help
> + Say N unless you are building kernels for fuzz testing.
> + Saying Y here disables several things that legitimately cause
> + damage under a fuzzer workload (e.g. copying to arbitrary
> + user-specified kernel address, changing console loglevel,
> + freezing filesystems).
> +
> endmenu # Kernel hacking
>
--
js
suse labs
On Sat, Mar 07, 2020 at 10:58:22PM +0900, Tetsuo Handa wrote:
> While syzkaller is finding many bugs, sometimes syzkaller examines
> stupid operations. Currently we prevent syzkaller from examining
> stupid operations by blacklisting syscall arguments and/or disabling
> whole functionality using existing kernel config options, but it is
> a whack-a-mole approach. We need cooperation from kernel side [1].
>
> This patch introduces a kernel config option which allows disabling
> only specific operations. This kernel config option should be enabled
> only when building kernels for fuzz testing.
>
> We discussed possibility of disabling specific operations at run-time
> using some lockdown mechanism [2], but conclusion seems that build-time
> control (i.e. kernel config option) fits better for this purpose.
> Since patches for users of this kernel config option will want a lot of
> explanation [3], this patch provides only kernel config option for them.
>
> [1] https://github.com/google/syzkaller/issues/1622
> [2] https://lkml.kernel.org/r/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com
> [3] https://lkml.kernel.org/r/[email protected]
>
> Signed-off-by: Tetsuo Handa <[email protected]>
> Cc: Dmitry Vyukov <[email protected]>
> ---
> lib/Kconfig.debug | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> Changes since v1:
> Drop users of this kernel config option.
> Update patch description.
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 53e786e0a604..e360090e24c5 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2208,4 +2208,14 @@ config HYPERV_TESTING
>
> endmenu # "Kernel Testing and Coverage"
>
> +config KERNEL_BUILT_FOR_FUZZ_TESTING
> + bool "Build kernel for fuzz testing"
> + default n
That's always the default, you can leave that.
> + help
No tabs for the above lines?
> + Say N unless you are building kernels for fuzz testing.
> + Saying Y here disables several things that legitimately cause
> + damage under a fuzzer workload (e.g. copying to arbitrary
> + user-specified kernel address, changing console loglevel,
> + freezing filesystems).
"cause damage under a fuzzer workload running as root."
And that's the issue here, the fuzzer wants to run as root and then do
things that are known to cause problems (poke at memory locations,
unmount filesystems, etc.) So you should describe why this is happening
and that it is to be expected :)
thanks,
greg k-h
On Sat, Mar 07, 2020 at 05:28:22PM +0100, Jiri Slaby wrote:
> On 07. 03. 20, 14:58, Tetsuo Handa wrote:
> > While syzkaller is finding many bugs, sometimes syzkaller examines
> > stupid operations. Currently we prevent syzkaller from examining
> > stupid operations by blacklisting syscall arguments and/or disabling
> > whole functionality using existing kernel config options, but it is
> > a whack-a-mole approach. We need cooperation from kernel side [1].
> >
> > This patch introduces a kernel config option which allows disabling
> > only specific operations. This kernel config option should be enabled
> > only when building kernels for fuzz testing.
> >
> > We discussed possibility of disabling specific operations at run-time
> > using some lockdown mechanism [2], but conclusion seems that build-time
> > control (i.e. kernel config option) fits better for this purpose.
> > Since patches for users of this kernel config option will want a lot of
> > explanation [3], this patch provides only kernel config option for them.
> >
> > [1] https://github.com/google/syzkaller/issues/1622
> > [2] https://lkml.kernel.org/r/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com
> > [3] https://lkml.kernel.org/r/[email protected]
> >
> > Signed-off-by: Tetsuo Handa <[email protected]>
> > Cc: Dmitry Vyukov <[email protected]>
> > ---
> > lib/Kconfig.debug | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > Changes since v1:
> > Drop users of this kernel config option.
> > Update patch description.
> >
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index 53e786e0a604..e360090e24c5 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -2208,4 +2208,14 @@ config HYPERV_TESTING
> >
> > endmenu # "Kernel Testing and Coverage"
> >
> > +config KERNEL_BUILT_FOR_FUZZ_TESTING
> > + bool "Build kernel for fuzz testing"
>
> If we really want to go this way, I wouldn't limit it for fuzz testing
> only. Static analyzers, symbolic executors, formal verifiers, etc. --
> all of them should avoid the paths.
No, anything that just evaluates the code should be fine, we want static
analyzers to be processing those code paths. Just not to run them as
root on a live system.
thanks,
greg k-h
On Sun, Mar 8, 2020 at 12:53 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> No, anything that just evaluates the code should be fine, we want static
> analyzers to be processing those code paths. Just not to run them as
> root on a live system.
So I can see the reason to run fuzz testing as root, but I have to
admit to hating the "special config option for this" approach.
I'd *much* rather see some way to just lock down certain things
individually. The patch in here just added the config option, which is
the least interesting part.
The things that that config option then would want to disable - those
are the things that maybe we want to have a way for the system admin
just generally say "disable this".
Nothing to do with fuzzing, imho.
Linus
On 2020/03/09 1:13, Linus Torvalds wrote:
> On Sun, Mar 8, 2020 at 12:53 AM Greg Kroah-Hartman
> <[email protected]> wrote:
>>
>> No, anything that just evaluates the code should be fine, we want static
>> analyzers to be processing those code paths. Just not to run them as
>> root on a live system.
>
> So I can see the reason to run fuzz testing as root, but I have to
> admit to hating the "special config option for this" approach.
>
> I'd *much* rather see some way to just lock down certain things
> individually. The patch in here just added the config option, which is
> the least interesting part.
I think that locking down individual thing using individual switch is an
endless game of maintaining list of switches. When someone adds a code
which should not be fuzzed, the author of that code or the maintainer of
fuzzers will add a new switch for that code, and the maintainer of fuzzers
forever has to follow new switches. I think that it is better to keep number
of switches minimal until we have to split into fine grained switches.
>
> The things that that config option then would want to disable - those
> are the things that maybe we want to have a way for the system admin
> just generally say "disable this".
>
> Nothing to do with fuzzing, imho.
>
> Linus
>
On Mon, 9 Mar 2020 20:22:47 +0900
Tetsuo Handa <[email protected]> wrote:
> I think that locking down individual thing using individual switch is an
> endless game of maintaining list of switches. When someone adds a code
> which should not be fuzzed, the author of that code or the maintainer of
> fuzzers will add a new switch for that code, and the maintainer of fuzzers
> forever has to follow new switches. I think that it is better to keep number
> of switches minimal until we have to split into fine grained switches.
Can't we add a "TESTING" or "FUZZING" lockdown switch, that keeps root from
executing things that shouldn't be fuzzed?
I highly doubt that a kernel developer would even think "this shouldn't be
fuzzed" when adding something. It's going to first be reported by the
fuzz testing anyway. Don't just push the burden to the kernel developers.
-- Steve
On 08. 03. 20, 7:52, Greg Kroah-Hartman wrote:
> On Sat, Mar 07, 2020 at 05:28:22PM +0100, Jiri Slaby wrote:
>> On 07. 03. 20, 14:58, Tetsuo Handa wrote:
>>> While syzkaller is finding many bugs, sometimes syzkaller examines
>>> stupid operations. Currently we prevent syzkaller from examining
>>> stupid operations by blacklisting syscall arguments and/or disabling
>>> whole functionality using existing kernel config options, but it is
>>> a whack-a-mole approach. We need cooperation from kernel side [1].
>>>
>>> This patch introduces a kernel config option which allows disabling
>>> only specific operations. This kernel config option should be enabled
>>> only when building kernels for fuzz testing.
>>>
>>> We discussed possibility of disabling specific operations at run-time
>>> using some lockdown mechanism [2], but conclusion seems that build-time
>>> control (i.e. kernel config option) fits better for this purpose.
>>> Since patches for users of this kernel config option will want a lot of
>>> explanation [3], this patch provides only kernel config option for them.
>>>
>>> [1] https://github.com/google/syzkaller/issues/1622
>>> [2] https://lkml.kernel.org/r/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com
>>> [3] https://lkml.kernel.org/r/[email protected]
>>>
>>> Signed-off-by: Tetsuo Handa <[email protected]>
>>> Cc: Dmitry Vyukov <[email protected]>
>>> ---
>>> lib/Kconfig.debug | 10 ++++++++++
>>> 1 file changed, 10 insertions(+)
>>>
>>> Changes since v1:
>>> Drop users of this kernel config option.
>>> Update patch description.
>>>
>>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>>> index 53e786e0a604..e360090e24c5 100644
>>> --- a/lib/Kconfig.debug
>>> +++ b/lib/Kconfig.debug
>>> @@ -2208,4 +2208,14 @@ config HYPERV_TESTING
>>>
>>> endmenu # "Kernel Testing and Coverage"
>>>
>>> +config KERNEL_BUILT_FOR_FUZZ_TESTING
>>> + bool "Build kernel for fuzz testing"
>>
>> If we really want to go this way, I wouldn't limit it for fuzz testing
>> only. Static analyzers, symbolic executors, formal verifiers, etc. --
>> all of them should avoid the paths.
>
> No, anything that just evaluates the code should be fine, we want static
> analyzers to be processing those code paths. Just not to run them as
> root on a live system.
Even static analyzers generate real-world counter-examples in .c. They
are ran dynamically to check if the issue is real or if it's only a
shortcoming of static analysis. Klee is one of those and I used to run
it on the kernel some time ago. Throwing such generated input results in
the same weird behavior as using fuzzy testing, while it's still not
fuzzy testing, but accurate testing.
thanks,
--
js
suse labs
On Mon, Mar 9, 2020 at 4:39 PM Jiri Slaby <[email protected]> wrote:
>
> On 08. 03. 20, 7:52, Greg Kroah-Hartman wrote:
> > On Sat, Mar 07, 2020 at 05:28:22PM +0100, Jiri Slaby wrote:
> >> On 07. 03. 20, 14:58, Tetsuo Handa wrote:
> >>> While syzkaller is finding many bugs, sometimes syzkaller examines
> >>> stupid operations. Currently we prevent syzkaller from examining
> >>> stupid operations by blacklisting syscall arguments and/or disabling
> >>> whole functionality using existing kernel config options, but it is
> >>> a whack-a-mole approach. We need cooperation from kernel side [1].
> >>>
> >>> This patch introduces a kernel config option which allows disabling
> >>> only specific operations. This kernel config option should be enabled
> >>> only when building kernels for fuzz testing.
> >>>
> >>> We discussed possibility of disabling specific operations at run-time
> >>> using some lockdown mechanism [2], but conclusion seems that build-time
> >>> control (i.e. kernel config option) fits better for this purpose.
> >>> Since patches for users of this kernel config option will want a lot of
> >>> explanation [3], this patch provides only kernel config option for them.
> >>>
> >>> [1] https://github.com/google/syzkaller/issues/1622
> >>> [2] https://lkml.kernel.org/r/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com
> >>> [3] https://lkml.kernel.org/r/[email protected]
> >>>
> >>> Signed-off-by: Tetsuo Handa <[email protected]>
> >>> Cc: Dmitry Vyukov <[email protected]>
> >>> ---
> >>> lib/Kconfig.debug | 10 ++++++++++
> >>> 1 file changed, 10 insertions(+)
> >>>
> >>> Changes since v1:
> >>> Drop users of this kernel config option.
> >>> Update patch description.
> >>>
> >>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> >>> index 53e786e0a604..e360090e24c5 100644
> >>> --- a/lib/Kconfig.debug
> >>> +++ b/lib/Kconfig.debug
> >>> @@ -2208,4 +2208,14 @@ config HYPERV_TESTING
> >>>
> >>> endmenu # "Kernel Testing and Coverage"
> >>>
> >>> +config KERNEL_BUILT_FOR_FUZZ_TESTING
> >>> + bool "Build kernel for fuzz testing"
> >>
> >> If we really want to go this way, I wouldn't limit it for fuzz testing
> >> only. Static analyzers, symbolic executors, formal verifiers, etc. --
> >> all of them should avoid the paths.
> >
> > No, anything that just evaluates the code should be fine, we want static
> > analyzers to be processing those code paths. Just not to run them as
> > root on a live system.
>
> Even static analyzers generate real-world counter-examples in .c. They
> are ran dynamically to check if the issue is real or if it's only a
> shortcoming of static analysis. Klee is one of those and I used to run
> it on the kernel some time ago. Throwing such generated input results in
> the same weird behavior as using fuzzy testing, while it's still not
> fuzzy testing, but accurate testing.
I am all for naming/framing this as a more generic option (good it at
least does not have SYZ in the name :)).
Re making it a single config vs a set of fine-grained configs. I think
making it fine-grained is a proper way to do it, but the point Tetsuo
raised is very real and painful as well -- when a kernel developer
adds another option, they will not go and update configs on all
external testing systems. This problem is also common for "enable all
boot tests that can run on this kernel", or "configure a 'standard'
debug build". Currently doing these things require all of expertise,
sacred knowledge, checking all configs one-by-one as well as checking
every new kernel patch and that needs to be done by everybody doing
any kernel testing.
I wonder if this can be solved by doing fine-grained configs, but also
adding some umbrella uber-config that will select all of the
individual options. Config system allows this, right? With "select" or
"default if" clauses. What would be better: have the umbrella option
select all individual, or all individual default to y if umbrella is
selected?
FTR, some of the things we would like to disable are collected here:
https://github.com/google/syzkaller/issues/1622
Steve, I am not sure if by lockdown you mean the existing lockdown
mechanism, or just something similar in nature. As Tetsuo pointed, the
possibility of using the existing lockdown mechanism for this was
discussed here (and rejected):
https://lore.kernel.org/lkml/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com/
On Tue, 10 Mar 2020 07:30:01 +0100
Dmitry Vyukov <[email protected]> wrote:
> Steve, I am not sure if by lockdown you mean the existing lockdown
> mechanism, or just something similar in nature. As Tetsuo pointed, the
> possibility of using the existing lockdown mechanism for this was
> discussed here (and rejected):
> https://lore.kernel.org/lkml/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com/
From Matthew's message a couple of emails earlier:
> Simplicity. Based on discussion, we didn't want the lockdown LSM to
> enable arbitrary combinations of lockdown primitives, both because
> that would make it extremely difficult for userland developers and
> because it would make it extremely easy for local admins to
> accidentally configure policies that didn't achieve the desired
> outcome. There's no inherent problem in adding new options, but really
> right now they should fall into cases where they're protecting either
> the integrity of the kernel or preventing leakage of confidential
> information from the kernel.
Now, if you are worried that fuzzing will cause harm, or crash the kernel,
it sounds to me, whatever fuzzing did would satisfy Matthew's "integrity of
the kernel" portion.
In other words, I believe fuzzing folks should be working with the lockdown
folks and letting the lockdown folks how root can crash the system. I would
think from a security point of view, that if there's a known method to take
down the kernel, and I don't want root to be able to do so, we should
either fix the kernel to not be able to do so, or if that interface is "you
should know what you are doing" then it should be something an admin could
lock down to keep other admins who don't know what they are doing from
crashing the system.
Or teach the fuzz tool not to do specific bad things.
Honestly, if you just go with a single config to prevent interfaces from
crashing the system while running a fuzz test, then you just lowered the
usefulness of the fuzz test, as it will never find legitimate cases where
that interface crashed the kernel when it should not have.
We are currently trying to clean up the tracing / probing code to not be
able to crash the kernel with any interface. It's hard, but it is a goal.
-- Steve
On Wed, Mar 11, 2020 at 3:11 PM Steven Rostedt <[email protected]> wrote:
>
> On Tue, 10 Mar 2020 07:30:01 +0100
> Dmitry Vyukov <[email protected]> wrote:
>
> > Steve, I am not sure if by lockdown you mean the existing lockdown
> > mechanism, or just something similar in nature. As Tetsuo pointed, the
> > possibility of using the existing lockdown mechanism for this was
> > discussed here (and rejected):
> > https://lore.kernel.org/lkml/CACdnJutc7OQeoor6WLTh8as10da_CN=crs79v3Fp0mJTaO=+yw@mail.gmail.com/
>
> From Matthew's message a couple of emails earlier:
>
> > Simplicity. Based on discussion, we didn't want the lockdown LSM to
> > enable arbitrary combinations of lockdown primitives, both because
> > that would make it extremely difficult for userland developers and
> > because it would make it extremely easy for local admins to
> > accidentally configure policies that didn't achieve the desired
> > outcome. There's no inherent problem in adding new options, but really
> > right now they should fall into cases where they're protecting either
> > the integrity of the kernel or preventing leakage of confidential
> > information from the kernel.
>
> Now, if you are worried that fuzzing will cause harm, or crash the kernel,
> it sounds to me, whatever fuzzing did would satisfy Matthew's "integrity of
> the kernel" portion.
>
> In other words, I believe fuzzing folks should be working with the lockdown
> folks and letting the lockdown folks how root can crash the system. I would
> think from a security point of view, that if there's a known method to take
> down the kernel, and I don't want root to be able to do so, we should
> either fix the kernel to not be able to do so, or if that interface is "you
> should know what you are doing" then it should be something an admin could
> lock down to keep other admins who don't know what they are doing from
> crashing the system.
The line crashing the system (for some definition) and
working-as-intended is somewhat fuzzy (no pun intended). E.g. FIFREEZE
is it crashing the system if used uncarefully? Or connecting a USB
keyboard programmatically (which can inject CTRL+ALT+DEL)? Or turning
off console output?
> Or teach the fuzz tool not to do specific bad things.
We do some of this.
But generally it's impossible for anything that involves memory
indirections, or depends on the exact type of fd (e.g. all ioctl's),
etc. Boils down to halting problem and ability to predict exact
behavior of arbitrary programs.
> Honestly, if you just go with a single config to prevent interfaces from
> crashing the system while running a fuzz test, then you just lowered the
> usefulness of the fuzz test, as it will never find legitimate cases where
> that interface crashed the kernel when it should not have.
I believe "coarse-grained" above refers to something different. We
don't disable whole subsystems (there are usually configs for that
already), but rather we have a single config that does small tweaks to
multiple subsystems (e.g. that single ioctl is disabled, etc). This
allows to still test majority of the subsystem, but just not that
single bit that is harmful.
> We are currently trying to clean up the tracing / probing code to not be
> able to crash the kernel with any interface. It's hard, but it is a goal.
>
> -- Steve
On 2020/03/13 4:23, Dmitry Vyukov wrote:
>> Or teach the fuzz tool not to do specific bad things.
>
> We do some of this.
> But generally it's impossible for anything that involves memory
> indirections, or depends on the exact type of fd (e.g. all ioctl's),
> etc. Boils down to halting problem and ability to predict exact
> behavior of arbitrary programs.
I would like to enable changes like below only if CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y .
Since TASK_RUNNING threads are not always running on CPUs (in syzbot, the kernel is
tested on a VM with only 2 CPUs, which means that many threads are simply waiting for
CPU time to be assigned), dumping locks held by all threads gives us more clue when
e.g. khungtask fired. But since lockdep_print_held_locks() is racy, I assume that
this change won't be accepted unless CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y .
Also, for another example, limit number of memory pages /dev/ion driver can consume only if
CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y ( https://github.com/google/syzkaller/issues/1267 ),
for limiting number of memory pages is a user-visible change while we need to avoid false
alarms caused by consuming all memory pages.
In other words, while majority of things CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y would
do "disable this", there would be a few "enable this" and "change this".
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 32406ef0d6a2..1bc7878768fc 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -695,6 +695,7 @@ static void print_lock(struct held_lock *hlock)
static void lockdep_print_held_locks(struct task_struct *p)
{
int i, depth = READ_ONCE(p->lockdep_depth);
+ bool unreliable;
if (!depth)
printk("no locks held by %s/%d.\n", p->comm, task_pid_nr(p));
@@ -705,10 +706,12 @@ static void lockdep_print_held_locks(struct task_struct *p)
* It's not reliable to print a task's held locks if it's not sleeping
* and it's not the current task.
*/
- if (p->state == TASK_RUNNING && p != current)
- return;
+ unreliable = p->state == TASK_RUNNING && p != current;
for (i = 0; i < depth; i++) {
- printk(" #%d: ", i);
+ if (unreliable)
+ printk(" #%d?: ", i);
+ else
+ printk(" #%d: ", i);
print_lock(p->held_locks + i);
}
}
On Fri, 13 Mar 2020 06:59:22 +0900
Tetsuo Handa <[email protected]> wrote:
> On 2020/03/13 4:23, Dmitry Vyukov wrote:
> >> Or teach the fuzz tool not to do specific bad things.
> >
> > We do some of this.
> > But generally it's impossible for anything that involves memory
> > indirections, or depends on the exact type of fd (e.g. all ioctl's),
> > etc. Boils down to halting problem and ability to predict exact
> > behavior of arbitrary programs.
>
> I would like to enable changes like below only if CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y .
>
> Since TASK_RUNNING threads are not always running on CPUs (in syzbot, the kernel is
> tested on a VM with only 2 CPUs, which means that many threads are simply waiting for
> CPU time to be assigned), dumping locks held by all threads gives us more clue when
> e.g. khungtask fired. But since lockdep_print_held_locks() is racy, I assume that
> this change won't be accepted unless CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y .
>
> Also, for another example, limit number of memory pages /dev/ion driver can consume only if
> CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y ( https://github.com/google/syzkaller/issues/1267 ),
> for limiting number of memory pages is a user-visible change while we need to avoid false
> alarms caused by consuming all memory pages.
>
> In other words, while majority of things CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y would
> do "disable this", there would be a few "enable this" and "change this".
I still fear that people will just disable large sections. I've seen this
before. Developers take the easy way out, and when someone adds a new
feature that may be dangerous, they will just say "oh turn off fuzzing" and
be done with it.
As Linus likes to say, when you need to make changes to the kernel to test
it, you are no longer testing production kernels.
>
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 32406ef0d6a2..1bc7878768fc 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -695,6 +695,7 @@ static void print_lock(struct held_lock *hlock)
> static void lockdep_print_held_locks(struct task_struct *p)
> {
> int i, depth = READ_ONCE(p->lockdep_depth);
> + bool unreliable;
>
> if (!depth)
> printk("no locks held by %s/%d.\n", p->comm, task_pid_nr(p));
> @@ -705,10 +706,12 @@ static void lockdep_print_held_locks(struct task_struct *p)
> * It's not reliable to print a task's held locks if it's not sleeping
> * and it's not the current task.
> */
> - if (p->state == TASK_RUNNING && p != current)
> - return;
> + unreliable = p->state == TASK_RUNNING && p != current;
> for (i = 0; i < depth; i++) {
> - printk(" #%d: ", i);
> + if (unreliable)
> + printk(" #%d?: ", i);
> + else
> + printk(" #%d: ", i);
Have you tried submitting this? Has Peter nacked it?
-- Steve
> print_lock(p->held_locks + i);
> }
> }
On Thu, Mar 12, 2020 at 06:29:35PM -0400, Steven Rostedt wrote:
> > @@ -705,10 +706,12 @@ static void lockdep_print_held_locks(struct task_struct *p)
> > * It's not reliable to print a task's held locks if it's not sleeping
> > * and it's not the current task.
> > */
> > - if (p->state == TASK_RUNNING && p != current)
> > - return;
> > + unreliable = p->state == TASK_RUNNING && p != current;
> > for (i = 0; i < depth; i++) {
> > - printk(" #%d: ", i);
> > + if (unreliable)
> > + printk(" #%d?: ", i);
> > + else
> > + printk(" #%d: ", i);
>
> Have you tried submitting this? Has Peter nacked it?
It has definite UaF potential... do we have a boot parameter that
signals the willingness to trade safetly for more debug output?
Over all, the risk of this going *bang* is quite low I think.
On Thu, Mar 12, 2020 at 11:29 PM Steven Rostedt <[email protected]> wrote:
>
> On Fri, 13 Mar 2020 06:59:22 +0900
> Tetsuo Handa <[email protected]> wrote:
>
> > On 2020/03/13 4:23, Dmitry Vyukov wrote:
> > >> Or teach the fuzz tool not to do specific bad things.
> > >
> > > We do some of this.
> > > But generally it's impossible for anything that involves memory
> > > indirections, or depends on the exact type of fd (e.g. all ioctl's),
> > > etc. Boils down to halting problem and ability to predict exact
> > > behavior of arbitrary programs.
> >
> > I would like to enable changes like below only if CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y .
> >
> > Since TASK_RUNNING threads are not always running on CPUs (in syzbot, the kernel is
> > tested on a VM with only 2 CPUs, which means that many threads are simply waiting for
> > CPU time to be assigned), dumping locks held by all threads gives us more clue when
> > e.g. khungtask fired. But since lockdep_print_held_locks() is racy, I assume that
> > this change won't be accepted unless CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y .
> >
> > Also, for another example, limit number of memory pages /dev/ion driver can consume only if
> > CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y ( https://github.com/google/syzkaller/issues/1267 ),
> > for limiting number of memory pages is a user-visible change while we need to avoid false
> > alarms caused by consuming all memory pages.
> >
> > In other words, while majority of things CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y would
> > do "disable this", there would be a few "enable this" and "change this".
>
> I still fear that people will just disable large sections. I've seen this
> before. Developers take the easy way out, and when someone adds a new
> feature that may be dangerous, they will just say "oh turn off fuzzing" and
> be done with it.
>
> As Linus likes to say, when you need to make changes to the kernel to test
> it, you are no longer testing production kernels.
Well, it's tradeoffs all the way down.
For example, KASAN also radically affects the kernel (changes just
every line of code). There still should be final testing of the actual
production build of course. But I would say generally one wants to
test with KASAN enabled all the time.
Or, it also depends on the baseline. If our baseline is "everybody is
crazy about testing, ready to prioritize it on top of everything else
and spend infinite amounts of time on it", then there may be better
solutions. But if our baseline is "no testing at all", or "we have to
disable whole subsystems b/c we don't have better realistic options
and only few people willing to spend at least some time on it", then
it may be a good practical solution ;)
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index 32406ef0d6a2..1bc7878768fc 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -695,6 +695,7 @@ static void print_lock(struct held_lock *hlock)
> > static void lockdep_print_held_locks(struct task_struct *p)
> > {
> > int i, depth = READ_ONCE(p->lockdep_depth);
> > + bool unreliable;
> >
> > if (!depth)
> > printk("no locks held by %s/%d.\n", p->comm, task_pid_nr(p));
> > @@ -705,10 +706,12 @@ static void lockdep_print_held_locks(struct task_struct *p)
> > * It's not reliable to print a task's held locks if it's not sleeping
> > * and it's not the current task.
> > */
> > - if (p->state == TASK_RUNNING && p != current)
> > - return;
> > + unreliable = p->state == TASK_RUNNING && p != current;
> > for (i = 0; i < depth; i++) {
> > - printk(" #%d: ", i);
> > + if (unreliable)
> > + printk(" #%d?: ", i);
> > + else
> > + printk(" #%d: ", i);
>
> Have you tried submitting this? Has Peter nacked it?
>
> -- Steve
>
> > print_lock(p->held_locks + i);
> > }
> > }
>
On 2020/03/10 15:30, Dmitry Vyukov wrote:
> Re making it a single config vs a set of fine-grained configs. I think
> making it fine-grained is a proper way to do it, but the point Tetsuo
> raised is very real and painful as well -- when a kernel developer
> adds another option, they will not go and update configs on all
> external testing systems. This problem is also common for "enable all
> boot tests that can run on this kernel", or "configure a 'standard'
> debug build". Currently doing these things require all of expertise,
> sacred knowledge, checking all configs one-by-one as well as checking
> every new kernel patch and that needs to be done by everybody doing
> any kernel testing.
> I wonder if this can be solved by doing fine-grained configs, but also
> adding some umbrella uber-config that will select all of the
> individual options. Config system allows this, right? With "select" or
> "default if" clauses. What would be better: have the umbrella option
> select all individual, or all individual default to y if umbrella is
> selected?
So, we have three questions.
Q1: Can we agree with adding build-time branching (i.e. kernel config options) ?
I fear bugs (e.g. unexpectedly overwrting flag variables) in run-time
branching mechanisms. Build-time branching mechanisms cannot have such bugs.
Q2: If we can agree with kernel config options, can we start with single (or
fewer) kernel config option (e.g. CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y) ?
Q3: If we can agree with kernel config options but we can't start with single
(or fewer) kernel config option, can we agree with adding another kernel
config option which selects all options (e.g.
config KERNEL_BUILT_FOR_FUZZ_TESTING
bool "Build kernel for fuzz testing"
config KERNEL_BUILT_FOR_FUZZ_TESTING_SELECT_ALL
bool "Select all options for Build kernel for fuzz testing"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
select KERNEL_DISABLE_FOO1
select KERNEL_DISABLE_BAR1
select KERNEL_DISABLE_BUZ1
select KERNEL_CHANGE_FOO2
select KERNEL_ENABLE_BAR2
config KERNEL_DISABLE_FOO1
bool "Disable foo1"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
config KERNEL_DISABLE_BAR1
bool "Disable bar1"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
config KERNEL_DISABLE_BUZ1
bool "Disable buz1"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
config KERNEL_CHANGE_FOO2
bool "Change foo2"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
config KERNEL_ENABLE_BAR2
bool "Enable bar2"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
) ?
On Sat, Mar 21, 2020 at 5:50 AM Tetsuo Handa
<[email protected]> wrote:
>
> On 2020/03/10 15:30, Dmitry Vyukov wrote:
> > Re making it a single config vs a set of fine-grained configs. I think
> > making it fine-grained is a proper way to do it, but the point Tetsuo
> > raised is very real and painful as well -- when a kernel developer
> > adds another option, they will not go and update configs on all
> > external testing systems. This problem is also common for "enable all
> > boot tests that can run on this kernel", or "configure a 'standard'
> > debug build". Currently doing these things require all of expertise,
> > sacred knowledge, checking all configs one-by-one as well as checking
> > every new kernel patch and that needs to be done by everybody doing
> > any kernel testing.
> > I wonder if this can be solved by doing fine-grained configs, but also
> > adding some umbrella uber-config that will select all of the
> > individual options. Config system allows this, right? With "select" or
> > "default if" clauses. What would be better: have the umbrella option
> > select all individual, or all individual default to y if umbrella is
> > selected?
>
> So, we have three questions.
>
> Q1: Can we agree with adding build-time branching (i.e. kernel config options) ?
>
> I fear bugs (e.g. unexpectedly overwrting flag variables) in run-time
> branching mechanisms. Build-time branching mechanisms cannot have such bugs.
My vote is for build config. It's simplest to configure (every testing
system should already have control over config) and most reliable
(e.g. fuzzer figures out a way to disable a runtime option).
> Q2: If we can agree with kernel config options, can we start with single (or
> fewer) kernel config option (e.g. CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y) ?
>
> Q3: If we can agree with kernel config options but we can't start with single
> (or fewer) kernel config option, can we agree with adding another kernel
> config option which selects all options (e.g.
>
> config KERNEL_BUILT_FOR_FUZZ_TESTING
> bool "Build kernel for fuzz testing"
>
> config KERNEL_BUILT_FOR_FUZZ_TESTING_SELECT_ALL
> bool "Select all options for Build kernel for fuzz testing"
> depends on KERNEL_BUILT_FOR_FUZZ_TESTING
> select KERNEL_DISABLE_FOO1
> select KERNEL_DISABLE_BAR1
> select KERNEL_DISABLE_BUZ1
> select KERNEL_CHANGE_FOO2
> select KERNEL_ENABLE_BAR2
>
> config KERNEL_DISABLE_FOO1
> bool "Disable foo1"
> depends on KERNEL_BUILT_FOR_FUZZ_TESTING
>
> config KERNEL_DISABLE_BAR1
> bool "Disable bar1"
> depends on KERNEL_BUILT_FOR_FUZZ_TESTING
>
> config KERNEL_DISABLE_BUZ1
> bool "Disable buz1"
> depends on KERNEL_BUILT_FOR_FUZZ_TESTING
>
> config KERNEL_CHANGE_FOO2
> bool "Change foo2"
> depends on KERNEL_BUILT_FOR_FUZZ_TESTING
>
> config KERNEL_ENABLE_BAR2
> bool "Enable bar2"
> depends on KERNEL_BUILT_FOR_FUZZ_TESTING
>
> ) ?
I think this option is the best. It both allows fine-grained control
(and documentation for each switch), and coarse-grained control for
testing systems.
We could also add "depends on DEBUG_KERNEL" just to make sure.
On 2020/03/24 19:37, Dmitry Vyukov wrote:
>> So, we have three questions.
>>
>> Q1: Can we agree with adding build-time branching (i.e. kernel config options) ?
>>
>> I fear bugs (e.g. unexpectedly overwrting flag variables) in run-time
>> branching mechanisms. Build-time branching mechanisms cannot have such bugs.
>
> My vote is for build config. It's simplest to configure (every testing
> system should already have control over config) and most reliable
> (e.g. fuzzer figures out a way to disable a runtime option).
Right. For example, console loglevel has been unexpectedly changed via syscall
arguments. For another example, /dev/mem has been unexpectedly accessed using
mount operation ( https://github.com/google/syzkaller/issues/1436 ). Providing
an interface (e.g. debugfs files) for branching after boot has a risk of
unexpectedly overwriting flag variables.
Since it seems that there is no objection to Q1, I think that we can go with
build-time branching.
>> Q3: If we can agree with kernel config options but we can't start with single
>> (or fewer) kernel config option, can we agree with adding another kernel
>> config option which selects all options (e.g.
>> ) ?
>
> I think this option is the best. It both allows fine-grained control
> (and documentation for each switch), and coarse-grained control for
> testing systems.
> We could also add "depends on DEBUG_KERNEL" just to make sure.
>
OK. But I think that KERNEL_BUILT_FOR_FUZZ_TESTING_SELECT_ALL might fail to
work, for it is possible that a new option was added but that option was not
added to "select" list of KERNEL_BUILT_FOR_FUZZ_TESTING_SELECT_ALL. Also,
there might be cases where a new option was added but that option should not
be selected for some fuzzers (e.g. syzkaller wants to hardcode console loglevel
to foo while fuzzer1 and fuzzer2 want to hardcode console loglevel to bar).
That is, something like
config KERNEL_BUILT_FOR_FUZZ_TESTING
bool "Build kernel for fuzz testing"
config KERNEL_BUILT_FOR_SYZKALLER
bool "Build kernel for syzkaller testing"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
select KERNEL_DISABLE_FOO1
select KERNEL_DISABLE_BAR1
select KERNEL_DISABLE_BUZ1
select KERNEL_ENABLE_BAR2
config KERNEL_BUILT_FOR_FUZZER1
bool "Build kernel for fuzzer1 testing"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
select KERNEL_DISABLE_FOO1
select KERNEL_DISABLE_BAR1
config KERNEL_BUILT_FOR_FUZZER2
bool "Build kernel for fuzzer2 testing"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
select KERNEL_DISABLE_FOO1
select KERNEL_DISABLE_BUZ1
config KERNEL_DISABLE_FOO1
bool "Disable foo1"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
config KERNEL_DISABLE_BAR1
bool "Disable bar1"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
config KERNEL_DISABLE_BUZ1
bool "Disable buz1"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
config KERNEL_CHANGE_FOO2
bool "Change foo2"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
config KERNEL_ENABLE_BAR2
bool "Enable bar2"
depends on KERNEL_BUILT_FOR_FUZZ_TESTING
in order to allow each testing system to select what it wants with
"only two options" ("CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING=y" and one of
"CONFIG_KERNEL_BUILT_FOR_SYZKALLER=y" or "CONFIG_KERNEL_BUILT_FOR_FUZZER1=y"
or "CONFIG_KERNEL_BUILT_FOR_FUZZER2=y").
CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING option remains there in order to
avoid needlessly prompting choices to users who do not intend to build
for fuzz testing.
Can we resume this topic?
On 2020/03/26 20:10, Tetsuo Handa wrote:
> CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING option remains there in order to
> avoid needlessly prompting choices to users who do not intend to build
> for fuzz testing.
>
Is CONFIG_TWIST_KERNEL_BEHAVIOR or CONFIG_TWEAK_KERNEL_BEHAVIOR
better-named than CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING, for there
might be small modifications which would be beneficial to things
other than fuzz testing (e.g. adding some more information when
dumping backtrace, reporting locks held by TASK_RUNNING threads) ?