* [PATCH v2 1/3] ubsan: Add trap instrumentation option
2019-11-21 18:15 [PATCH v2 0/3] ubsan: Split out bounds checker Kees Cook
@ 2019-11-21 18:15 ` Kees Cook
2019-12-16 10:26 ` Will Deacon
2019-11-21 18:15 ` [PATCH v2 2/3] ubsan: Split "bounds" checker from other options Kees Cook
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Kees Cook @ 2019-11-21 18:15 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Andrey Ryabinin, Elena Petrova, Alexander Potapenko,
Dmitry Vyukov, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
linux-kernel, kernel-hardening
The Undefined Behavior Sanitizer can operate in two modes: warning
reporting mode via lib/ubsan.c handler calls, or trap mode, which uses
__builtin_trap() as the handler. Using lib/ubsan.c means the kernel
image is about 5% larger (due to all the debugging text and reporting
structures to capture details about the warning conditions). Using the
trap mode, the image size changes are much smaller, though at the loss
of the "warning only" mode.
In order to give greater flexibility to system builders that want
minimal changes to image size and are prepared to deal with kernel code
being aborted and potentially destabilizing the system, this introduces
CONFIG_UBSAN_TRAP. The resulting image sizes comparison:
text data bss dec hex filename
19533663 6183037 18554956 44271656 2a38828 vmlinux.stock
19991849 7618513 18874448 46484810 2c54d4a vmlinux.ubsan
19712181 6284181 18366540 44362902 2a4ec96 vmlinux.ubsan-trap
CONFIG_UBSAN=y: image +4.8% (text +2.3%, data +18.9%)
CONFIG_UBSAN_TRAP=y: image +0.2% (text +0.9%, data +1.6%)
Additionally adjusts the CONFIG_UBSAN Kconfig help for clarity and
removes the mention of non-existing boot param "ubsan_handle".
Suggested-by: Elena Petrova <lenaptr@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
lib/Kconfig.ubsan | 22 ++++++++++++++++++----
lib/Makefile | 2 ++
scripts/Makefile.ubsan | 9 +++++++--
3 files changed, 27 insertions(+), 6 deletions(-)
diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index 0e04fcb3ab3d..9deb655838b0 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -5,11 +5,25 @@ config ARCH_HAS_UBSAN_SANITIZE_ALL
config UBSAN
bool "Undefined behaviour sanity checker"
help
- This option enables undefined behaviour sanity checker
+ This option enables the Undefined Behaviour sanity checker.
Compile-time instrumentation is used to detect various undefined
- behaviours in runtime. Various types of checks may be enabled
- via boot parameter ubsan_handle
- (see: Documentation/dev-tools/ubsan.rst).
+ behaviours at runtime. For more details, see:
+ Documentation/dev-tools/ubsan.rst
+
+config UBSAN_TRAP
+ bool "On Sanitizer warnings, abort the running kernel code"
+ depends on UBSAN
+ depends on $(cc-option, -fsanitize-undefined-trap-on-error)
+ help
+ Building kernels with Sanitizer features enabled tends to grow
+ the kernel size by around 5%, due to adding all the debugging
+ text on failure paths. To avoid this, Sanitizer instrumentation
+ can just issue a trap. This reduces the kernel size overhead but
+ turns all warnings (including potentially harmless conditions)
+ into full exceptions that abort the running kernel code
+ (regardless of context, locks held, etc), which may destabilize
+ the system. For some system builders this is an acceptable
+ trade-off.
config UBSAN_SANITIZE_ALL
bool "Enable instrumentation for the entire kernel"
diff --git a/lib/Makefile b/lib/Makefile
index c5892807e06f..bc498bf0f52d 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -272,7 +272,9 @@ quiet_cmd_build_OID_registry = GEN $@
clean-files += oid_registry_data.c
obj-$(CONFIG_UCS2_STRING) += ucs2_string.o
+ifneq ($(CONFIG_UBSAN_TRAP),y)
obj-$(CONFIG_UBSAN) += ubsan.o
+endif
UBSAN_SANITIZE_ubsan.o := n
KASAN_SANITIZE_ubsan.o := n
diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
index 019771b845c5..668a91510bfe 100644
--- a/scripts/Makefile.ubsan
+++ b/scripts/Makefile.ubsan
@@ -1,5 +1,10 @@
# SPDX-License-Identifier: GPL-2.0
ifdef CONFIG_UBSAN
+
+ifdef CONFIG_UBSAN_ALIGNMENT
+ CFLAGS_UBSAN += $(call cc-option, -fsanitize=alignment)
+endif
+
CFLAGS_UBSAN += $(call cc-option, -fsanitize=shift)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=integer-divide-by-zero)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=unreachable)
@@ -9,8 +14,8 @@ ifdef CONFIG_UBSAN
CFLAGS_UBSAN += $(call cc-option, -fsanitize=bool)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=enum)
-ifdef CONFIG_UBSAN_ALIGNMENT
- CFLAGS_UBSAN += $(call cc-option, -fsanitize=alignment)
+ifdef CONFIG_UBSAN_TRAP
+ CFLAGS_UBSAN += $(call cc-option, -fsanitize-undefined-trap-on-error)
endif
# -fsanitize=* options makes GCC less smart than usual and
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 1/3] ubsan: Add trap instrumentation option
2019-11-21 18:15 ` [PATCH v2 1/3] ubsan: Add trap instrumentation option Kees Cook
@ 2019-12-16 10:26 ` Will Deacon
2019-12-18 0:08 ` Kees Cook
0 siblings, 1 reply; 15+ messages in thread
From: Will Deacon @ 2019-12-16 10:26 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Dmitry Vyukov, Linus Torvalds,
Dan Carpenter, Gustavo A. R. Silva, Arnd Bergmann,
Ard Biesheuvel, kasan-dev, linux-kernel, kernel-hardening
Hi Kees,
On Thu, Nov 21, 2019 at 10:15:17AM -0800, Kees Cook wrote:
> The Undefined Behavior Sanitizer can operate in two modes: warning
> reporting mode via lib/ubsan.c handler calls, or trap mode, which uses
> __builtin_trap() as the handler. Using lib/ubsan.c means the kernel
> image is about 5% larger (due to all the debugging text and reporting
> structures to capture details about the warning conditions). Using the
> trap mode, the image size changes are much smaller, though at the loss
> of the "warning only" mode.
>
> In order to give greater flexibility to system builders that want
> minimal changes to image size and are prepared to deal with kernel code
> being aborted and potentially destabilizing the system, this introduces
> CONFIG_UBSAN_TRAP. The resulting image sizes comparison:
>
> text data bss dec hex filename
> 19533663 6183037 18554956 44271656 2a38828 vmlinux.stock
> 19991849 7618513 18874448 46484810 2c54d4a vmlinux.ubsan
> 19712181 6284181 18366540 44362902 2a4ec96 vmlinux.ubsan-trap
>
> CONFIG_UBSAN=y: image +4.8% (text +2.3%, data +18.9%)
> CONFIG_UBSAN_TRAP=y: image +0.2% (text +0.9%, data +1.6%)
>
> Additionally adjusts the CONFIG_UBSAN Kconfig help for clarity and
> removes the mention of non-existing boot param "ubsan_handle".
>
> Suggested-by: Elena Petrova <lenaptr@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> lib/Kconfig.ubsan | 22 ++++++++++++++++++----
> lib/Makefile | 2 ++
> scripts/Makefile.ubsan | 9 +++++++--
> 3 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> index 0e04fcb3ab3d..9deb655838b0 100644
> --- a/lib/Kconfig.ubsan
> +++ b/lib/Kconfig.ubsan
> @@ -5,11 +5,25 @@ config ARCH_HAS_UBSAN_SANITIZE_ALL
> config UBSAN
> bool "Undefined behaviour sanity checker"
> help
> - This option enables undefined behaviour sanity checker
> + This option enables the Undefined Behaviour sanity checker.
> Compile-time instrumentation is used to detect various undefined
> - behaviours in runtime. Various types of checks may be enabled
> - via boot parameter ubsan_handle
> - (see: Documentation/dev-tools/ubsan.rst).
> + behaviours at runtime. For more details, see:
> + Documentation/dev-tools/ubsan.rst
> +
> +config UBSAN_TRAP
> + bool "On Sanitizer warnings, abort the running kernel code"
> + depends on UBSAN
> + depends on $(cc-option, -fsanitize-undefined-trap-on-error)
> + help
> + Building kernels with Sanitizer features enabled tends to grow
> + the kernel size by around 5%, due to adding all the debugging
> + text on failure paths. To avoid this, Sanitizer instrumentation
> + can just issue a trap. This reduces the kernel size overhead but
> + turns all warnings (including potentially harmless conditions)
> + into full exceptions that abort the running kernel code
> + (regardless of context, locks held, etc), which may destabilize
> + the system. For some system builders this is an acceptable
> + trade-off.
Slight nit, but I wonder if it would make sense to move all this under a
'menuconfig UBSAN' entry, so the dependencies can be dropped? Then you could
have all of the suboptions default to on and basically choose which
individual compiler options to disable based on your own preferences.
What do you think?
Will
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/3] ubsan: Add trap instrumentation option
2019-12-16 10:26 ` Will Deacon
@ 2019-12-18 0:08 ` Kees Cook
0 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2019-12-18 0:08 UTC (permalink / raw)
To: Will Deacon
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Dmitry Vyukov, Linus Torvalds,
Dan Carpenter, Gustavo A. R. Silva, Arnd Bergmann,
Ard Biesheuvel, kasan-dev, linux-kernel, kernel-hardening
On Mon, Dec 16, 2019 at 10:26:56AM +0000, Will Deacon wrote:
> Hi Kees,
>
> On Thu, Nov 21, 2019 at 10:15:17AM -0800, Kees Cook wrote:
> > The Undefined Behavior Sanitizer can operate in two modes: warning
> > reporting mode via lib/ubsan.c handler calls, or trap mode, which uses
> > __builtin_trap() as the handler. Using lib/ubsan.c means the kernel
> > image is about 5% larger (due to all the debugging text and reporting
> > structures to capture details about the warning conditions). Using the
> > trap mode, the image size changes are much smaller, though at the loss
> > of the "warning only" mode.
> >
> > In order to give greater flexibility to system builders that want
> > minimal changes to image size and are prepared to deal with kernel code
> > being aborted and potentially destabilizing the system, this introduces
> > CONFIG_UBSAN_TRAP. The resulting image sizes comparison:
> >
> > text data bss dec hex filename
> > 19533663 6183037 18554956 44271656 2a38828 vmlinux.stock
> > 19991849 7618513 18874448 46484810 2c54d4a vmlinux.ubsan
> > 19712181 6284181 18366540 44362902 2a4ec96 vmlinux.ubsan-trap
> >
> > CONFIG_UBSAN=y: image +4.8% (text +2.3%, data +18.9%)
> > CONFIG_UBSAN_TRAP=y: image +0.2% (text +0.9%, data +1.6%)
> >
> > Additionally adjusts the CONFIG_UBSAN Kconfig help for clarity and
> > removes the mention of non-existing boot param "ubsan_handle".
> >
> > Suggested-by: Elena Petrova <lenaptr@google.com>
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > lib/Kconfig.ubsan | 22 ++++++++++++++++++----
> > lib/Makefile | 2 ++
> > scripts/Makefile.ubsan | 9 +++++++--
> > 3 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
> > index 0e04fcb3ab3d..9deb655838b0 100644
> > --- a/lib/Kconfig.ubsan
> > +++ b/lib/Kconfig.ubsan
> > @@ -5,11 +5,25 @@ config ARCH_HAS_UBSAN_SANITIZE_ALL
> > config UBSAN
> > bool "Undefined behaviour sanity checker"
> > help
> > - This option enables undefined behaviour sanity checker
> > + This option enables the Undefined Behaviour sanity checker.
> > Compile-time instrumentation is used to detect various undefined
> > - behaviours in runtime. Various types of checks may be enabled
> > - via boot parameter ubsan_handle
> > - (see: Documentation/dev-tools/ubsan.rst).
> > + behaviours at runtime. For more details, see:
> > + Documentation/dev-tools/ubsan.rst
> > +
> > +config UBSAN_TRAP
> > + bool "On Sanitizer warnings, abort the running kernel code"
> > + depends on UBSAN
> > + depends on $(cc-option, -fsanitize-undefined-trap-on-error)
> > + help
> > + Building kernels with Sanitizer features enabled tends to grow
> > + the kernel size by around 5%, due to adding all the debugging
> > + text on failure paths. To avoid this, Sanitizer instrumentation
> > + can just issue a trap. This reduces the kernel size overhead but
> > + turns all warnings (including potentially harmless conditions)
> > + into full exceptions that abort the running kernel code
> > + (regardless of context, locks held, etc), which may destabilize
> > + the system. For some system builders this is an acceptable
> > + trade-off.
>
> Slight nit, but I wonder if it would make sense to move all this under a
> 'menuconfig UBSAN' entry, so the dependencies can be dropped? Then you could
> have all of the suboptions default to on and basically choose which
> individual compiler options to disable based on your own preferences.
Sure; I can do that. I'll respin the series.
--
Kees Cook
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/3] ubsan: Split "bounds" checker from other options
2019-11-21 18:15 [PATCH v2 0/3] ubsan: Split out bounds checker Kees Cook
2019-11-21 18:15 ` [PATCH v2 1/3] ubsan: Add trap instrumentation option Kees Cook
@ 2019-11-21 18:15 ` Kees Cook
2019-11-21 18:15 ` [PATCH v2 3/3] lkdtm/bugs: Add arithmetic overflow and array bounds checks Kees Cook
2019-11-22 9:07 ` [PATCH v2 0/3] ubsan: Split out bounds checker Dmitry Vyukov
3 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2019-11-21 18:15 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Andrey Ryabinin, Elena Petrova, Alexander Potapenko,
Dmitry Vyukov, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
linux-kernel, kernel-hardening
In order to do kernel builds with the bounds checker individually
available, introduce CONFIG_UBSAN_BOUNDS, with the remaining options
under CONFIG_UBSAN_MISC.
For example, using this, we can start to expand the coverage syzkaller is
providing. Right now, all of UBSan is disabled for syzbot builds because
taken as a whole, it is too noisy. This will let us focus on one feature
at a time.
For the bounds checker specifically, this provides a mechanism to
eliminate an entire class of array overflows with close to zero
performance overhead (I cannot measure a difference). In my (mostly)
defconfig, enabling bounds checking adds ~4200 checks to the kernel.
Performance changes are in the noise, likely due to the branch predictors
optimizing for the non-fail path.
Some notes on the bounds checker:
- it does not instrument {mem,str}*()-family functions, it only
instruments direct indexed accesses (e.g. "foo[i]"). Dealing with
the {mem,str}*()-family functions is a work-in-progress around
CONFIG_FORTIFY_SOURCE[1].
- it ignores flexible array members, including the very old single
byte (e.g. "int foo[1];") declarations. (Note that GCC's
implementation appears to ignore _all_ trailing arrays, but Clang only
ignores empty, 0, and 1 byte arrays[2].)
[1] https://github.com/KSPP/linux/issues/6
[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92589
Suggested-by: Elena Petrova <lenaptr@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
lib/Kconfig.ubsan | 20 ++++++++++++++++++++
scripts/Makefile.ubsan | 7 ++++++-
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/lib/Kconfig.ubsan b/lib/Kconfig.ubsan
index 9deb655838b0..9b9f76d1a3f7 100644
--- a/lib/Kconfig.ubsan
+++ b/lib/Kconfig.ubsan
@@ -25,6 +25,26 @@ config UBSAN_TRAP
the system. For some system builders this is an acceptable
trade-off.
+config UBSAN_BOUNDS
+ bool "Perform array index bounds checking"
+ depends on UBSAN
+ default UBSAN
+ help
+ This option enables detection of directly indexed out of bounds
+ array accesses, where the array size is known at compile time.
+ Note that this does not protect array overflows via bad calls
+ to the {str,mem}*cpy() family of functions (that is addressed
+ by CONFIG_FORTIFY_SOURCE).
+
+config UBSAN_MISC
+ bool "Enable all other Undefined Behavior sanity checks"
+ depends on UBSAN
+ default UBSAN
+ help
+ This option enables all sanity checks that don't have their
+ own Kconfig options. Disable this if you only want to have
+ individually selected checks.
+
config UBSAN_SANITIZE_ALL
bool "Enable instrumentation for the entire kernel"
depends on UBSAN
diff --git a/scripts/Makefile.ubsan b/scripts/Makefile.ubsan
index 668a91510bfe..5b15bc425ec9 100644
--- a/scripts/Makefile.ubsan
+++ b/scripts/Makefile.ubsan
@@ -5,14 +5,19 @@ ifdef CONFIG_UBSAN_ALIGNMENT
CFLAGS_UBSAN += $(call cc-option, -fsanitize=alignment)
endif
+ifdef CONFIG_UBSAN_BOUNDS
+ CFLAGS_UBSAN += $(call cc-option, -fsanitize=bounds)
+endif
+
+ifdef CONFIG_UBSAN_MISC
CFLAGS_UBSAN += $(call cc-option, -fsanitize=shift)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=integer-divide-by-zero)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=unreachable)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=signed-integer-overflow)
- CFLAGS_UBSAN += $(call cc-option, -fsanitize=bounds)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=object-size)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=bool)
CFLAGS_UBSAN += $(call cc-option, -fsanitize=enum)
+endif
ifdef CONFIG_UBSAN_TRAP
CFLAGS_UBSAN += $(call cc-option, -fsanitize-undefined-trap-on-error)
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v2 3/3] lkdtm/bugs: Add arithmetic overflow and array bounds checks
2019-11-21 18:15 [PATCH v2 0/3] ubsan: Split out bounds checker Kees Cook
2019-11-21 18:15 ` [PATCH v2 1/3] ubsan: Add trap instrumentation option Kees Cook
2019-11-21 18:15 ` [PATCH v2 2/3] ubsan: Split "bounds" checker from other options Kees Cook
@ 2019-11-21 18:15 ` Kees Cook
2019-11-22 9:07 ` [PATCH v2 0/3] ubsan: Split out bounds checker Dmitry Vyukov
3 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2019-11-21 18:15 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Andrey Ryabinin, Elena Petrova, Alexander Potapenko,
Dmitry Vyukov, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
linux-kernel, kernel-hardening
Adds LKDTM tests for arithmetic overflow (both signed and unsigned),
as well as array bounds checking.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/misc/lkdtm/bugs.c | 75 ++++++++++++++++++++++++++++++++++++++
drivers/misc/lkdtm/core.c | 3 ++
drivers/misc/lkdtm/lkdtm.h | 3 ++
3 files changed, 81 insertions(+)
diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index 7284a22b1a09..8b4ef30f53c6 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -11,6 +11,7 @@
#include <linux/sched/signal.h>
#include <linux/sched/task_stack.h>
#include <linux/uaccess.h>
+#include <linux/slab.h>
struct lkdtm_list {
struct list_head node;
@@ -171,6 +172,80 @@ void lkdtm_HUNG_TASK(void)
schedule();
}
+volatile unsigned int huge = INT_MAX - 2;
+volatile unsigned int ignored;
+
+void lkdtm_OVERFLOW_SIGNED(void)
+{
+ int value;
+
+ value = huge;
+ pr_info("Normal signed addition ...\n");
+ value += 1;
+ ignored = value;
+
+ pr_info("Overflowing signed addition ...\n");
+ value += 4;
+ ignored = value;
+}
+
+
+void lkdtm_OVERFLOW_UNSIGNED(void)
+{
+ unsigned int value;
+
+ value = huge;
+ pr_info("Normal unsigned addition ...\n");
+ value += 1;
+ ignored = value;
+
+ pr_info("Overflowing unsigned addition ...\n");
+ value += 4;
+ ignored = value;
+}
+
+/* Intentially using old-style flex array definition of 1 byte. */
+struct array_bounds_flex_array {
+ int one;
+ int two;
+ char data[1];
+};
+
+struct array_bounds {
+ int one;
+ int two;
+ char data[8];
+ int three;
+};
+
+void lkdtm_ARRAY_BOUNDS(void)
+{
+ struct array_bounds_flex_array *not_checked;
+ struct array_bounds *checked;
+ int i;
+
+ not_checked = kmalloc(sizeof(*not_checked) * 2, GFP_KERNEL);
+ checked = kmalloc(sizeof(*checked) * 2, GFP_KERNEL);
+
+ pr_info("Array access within bounds ...\n");
+ /* For both, touch all bytes in the actual member size. */
+ for (i = 0; i < sizeof(checked->data); i++)
+ checked->data[i] = 'A';
+ /*
+ * For the uninstrumented flex array member, also touch 1 byte
+ * beyond to verify it is correctly uninstrumented.
+ */
+ for (i = 0; i < sizeof(not_checked->data) + 1; i++)
+ not_checked->data[i] = 'A';
+
+ pr_info("Array access beyond bounds ...\n");
+ for (i = 0; i < sizeof(checked->data) + 1; i++)
+ checked->data[i] = 'B';
+
+ kfree(not_checked);
+ kfree(checked);
+}
+
void lkdtm_CORRUPT_LIST_ADD(void)
{
/*
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index cbc4c9045a99..25879f7b0768 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -129,6 +129,9 @@ static const struct crashtype crashtypes[] = {
CRASHTYPE(HARDLOCKUP),
CRASHTYPE(SPINLOCKUP),
CRASHTYPE(HUNG_TASK),
+ CRASHTYPE(OVERFLOW_SIGNED),
+ CRASHTYPE(OVERFLOW_UNSIGNED),
+ CRASHTYPE(ARRAY_BOUNDS),
CRASHTYPE(EXEC_DATA),
CRASHTYPE(EXEC_STACK),
CRASHTYPE(EXEC_KMALLOC),
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index ab446e0bde97..2cd0c5031eea 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -22,6 +22,9 @@ void lkdtm_SOFTLOCKUP(void);
void lkdtm_HARDLOCKUP(void);
void lkdtm_SPINLOCKUP(void);
void lkdtm_HUNG_TASK(void);
+void lkdtm_OVERFLOW_SIGNED(void);
+void lkdtm_OVERFLOW_UNSIGNED(void);
+void lkdtm_ARRAY_BOUNDS(void);
void lkdtm_CORRUPT_LIST_ADD(void);
void lkdtm_CORRUPT_LIST_DEL(void);
void lkdtm_CORRUPT_USER_DS(void);
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-21 18:15 [PATCH v2 0/3] ubsan: Split out bounds checker Kees Cook
` (2 preceding siblings ...)
2019-11-21 18:15 ` [PATCH v2 3/3] lkdtm/bugs: Add arithmetic overflow and array bounds checks Kees Cook
@ 2019-11-22 9:07 ` Dmitry Vyukov
2019-11-22 16:52 ` Kees Cook
2019-11-27 5:42 ` Kees Cook
3 siblings, 2 replies; 15+ messages in thread
From: Dmitry Vyukov @ 2019-11-22 9:07 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
LKML, kernel-hardening, syzkaller
On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@chromium.org> wrote:
>
> v2:
> - clarify Kconfig help text (aryabinin)
> - add reviewed-by
> - aim series at akpm, which seems to be where ubsan goes through?
> v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@chromium.org
>
> This splits out the bounds checker so it can be individually used. This
> is expected to be enabled in Android and hopefully for syzbot. Includes
> LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
>
> -Kees
+syzkaller mailing list
This is great!
I wanted to enable UBSAN on syzbot for a long time. And it's
_probably_ not lots of work. But it was stuck on somebody actually
dedicating some time specifically for it.
Kees, or anybody else interested, could you provide relevant configs
that (1) useful for kernel, (2) we want 100% cleanliness, (3) don't
fire all the time even without fuzzing? Anything else required to
enable UBSAN? I don't see anything. syzbot uses gcc 8.something, which
I assume should be enough (but we can upgrade if necessary).
> Kees Cook (3):
> ubsan: Add trap instrumentation option
> ubsan: Split "bounds" checker from other options
> lkdtm/bugs: Add arithmetic overflow and array bounds checks
>
> drivers/misc/lkdtm/bugs.c | 75 ++++++++++++++++++++++++++++++++++++++
> drivers/misc/lkdtm/core.c | 3 ++
> drivers/misc/lkdtm/lkdtm.h | 3 ++
> lib/Kconfig.ubsan | 42 +++++++++++++++++++--
> lib/Makefile | 2 +
> scripts/Makefile.ubsan | 16 ++++++--
> 6 files changed, 134 insertions(+), 7 deletions(-)
>
> --
> 2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-22 9:07 ` [PATCH v2 0/3] ubsan: Split out bounds checker Dmitry Vyukov
@ 2019-11-22 16:52 ` Kees Cook
2019-11-27 5:42 ` Kees Cook
1 sibling, 0 replies; 15+ messages in thread
From: Kees Cook @ 2019-11-22 16:52 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
LKML, kernel-hardening, syzkaller
On Fri, Nov 22, 2019 at 10:07:29AM +0100, Dmitry Vyukov wrote:
> On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > v2:
> > - clarify Kconfig help text (aryabinin)
> > - add reviewed-by
> > - aim series at akpm, which seems to be where ubsan goes through?
> > v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@chromium.org
> >
> > This splits out the bounds checker so it can be individually used. This
> > is expected to be enabled in Android and hopefully for syzbot. Includes
> > LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
> >
> > -Kees
>
> +syzkaller mailing list
>
> This is great!
>
> I wanted to enable UBSAN on syzbot for a long time. And it's
> _probably_ not lots of work. But it was stuck on somebody actually
> dedicating some time specifically for it.
> Kees, or anybody else interested, could you provide relevant configs
> that (1) useful for kernel, (2) we want 100% cleanliness, (3) don't
> fire all the time even without fuzzing? Anything else required to
> enable UBSAN? I don't see anything. syzbot uses gcc 8.something, which
> I assume should be enough (but we can upgrade if necessary).
Nothing external should be needed; GCC and Clang support the ubsan
options. Once this series lands, it should be possible to just enable
this with:
CONFIG_UBSAN=y
CONFIG_UBSAN_BOUNDS=y
# CONFIG_UBSAN_MISC is not set
Based on initial testing, the bounds checker isn't very noisy, but I
haven't spun up a syzbot instance to really confirm this yet (that was
on the TODO list for today to let it run over the weekend).
--
Kees Cook
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-22 9:07 ` [PATCH v2 0/3] ubsan: Split out bounds checker Dmitry Vyukov
2019-11-22 16:52 ` Kees Cook
@ 2019-11-27 5:42 ` Kees Cook
2019-11-27 6:54 ` Dmitry Vyukov
1 sibling, 1 reply; 15+ messages in thread
From: Kees Cook @ 2019-11-27 5:42 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
LKML, kernel-hardening, syzkaller
On Fri, Nov 22, 2019 at 10:07:29AM +0100, Dmitry Vyukov wrote:
> On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > v2:
> > - clarify Kconfig help text (aryabinin)
> > - add reviewed-by
> > - aim series at akpm, which seems to be where ubsan goes through?
> > v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@chromium.org
> >
> > This splits out the bounds checker so it can be individually used. This
> > is expected to be enabled in Android and hopefully for syzbot. Includes
> > LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
> >
> > -Kees
>
> +syzkaller mailing list
>
> This is great!
BTW, can I consider this your Acked-by for these patches? :)
> I wanted to enable UBSAN on syzbot for a long time. And it's
> _probably_ not lots of work. But it was stuck on somebody actually
> dedicating some time specifically for it.
Do you have a general mechanism to test that syzkaller will actually
pick up the kernel log splat of a new check? I noticed a few things
about the ubsan handlers: they don't use any of the common "warn"
infrastructure (neither does kasan from what I can see), and was missing
a check for panic_on_warn (kasan has this, but does it incorrectly).
I think kasan and ubsan should be reworked to use the common warn
infrastructure, and at the very least, ubsan needs this:
diff --git a/lib/ubsan.c b/lib/ubsan.c
index e7d31735950d..a2535a62c9af 100644
--- a/lib/ubsan.c
+++ b/lib/ubsan.c
@@ -160,6 +160,17 @@ static void ubsan_epilogue(unsigned long *flags)
"========================================\n");
spin_unlock_irqrestore(&report_lock, *flags);
current->in_ubsan--;
+
+ if (panic_on_warn) {
+ /*
+ * This thread may hit another WARN() in the panic path.
+ * Resetting this prevents additional WARN() from panicking the
+ * system on this thread. Other threads are blocked by the
+ * panic_mutex in panic().
+ */
+ panic_on_warn = 0;
+ panic("panic_on_warn set ...\n");
+ }
}
static void handle_overflow(struct overflow_data *data, void *lhs,
> Kees, or anybody else interested, could you provide relevant configs
> that (1) useful for kernel,
As mentioned in the other email (but just to keep the note together with
the other thoughts here) after this series, you'd want:
CONFIG_UBSAN=y
CONFIG_UBSAN_BOUNDS=y
# CONFIG_UBSAN_MISC is not set
> (2) we want 100% cleanliness,
What do you mean here by "cleanliness"? It seems different from (3)
about the test tripping a lot?
> (3) don't
> fire all the time even without fuzzing?
I ran with the bounds checker enabled (and the above patch) under
syzkaller for the weekend and saw 0 bounds checker reports.
> Anything else required to
> enable UBSAN? I don't see anything. syzbot uses gcc 8.something, which
> I assume should be enough (but we can upgrade if necessary).
As mentioned, gcc 8+ should be fine.
--
Kees Cook
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-27 5:42 ` Kees Cook
@ 2019-11-27 6:54 ` Dmitry Vyukov
2019-11-27 9:34 ` Dmitry Vyukov
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2019-11-27 6:54 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
LKML, kernel-hardening, syzkaller
On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Nov 22, 2019 at 10:07:29AM +0100, Dmitry Vyukov wrote:
> > On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > v2:
> > > - clarify Kconfig help text (aryabinin)
> > > - add reviewed-by
> > > - aim series at akpm, which seems to be where ubsan goes through?
> > > v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@chromium.org
> > >
> > > This splits out the bounds checker so it can be individually used. This
> > > is expected to be enabled in Android and hopefully for syzbot. Includes
> > > LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
> > >
> > > -Kees
> >
> > +syzkaller mailing list
> >
> > This is great!
>
> BTW, can I consider this your Acked-by for these patches? :)
>
> > I wanted to enable UBSAN on syzbot for a long time. And it's
> > _probably_ not lots of work. But it was stuck on somebody actually
> > dedicating some time specifically for it.
>
> Do you have a general mechanism to test that syzkaller will actually
> pick up the kernel log splat of a new check?
Yes. That's one of the most important and critical parts of syzkaller :)
The tests for different types of bugs are here:
https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report
But have 3 for UBSAN, but they may be old and it would be useful to
have 1 example crash per bug type:
syzkaller$ grep UBSAN pkg/report/testdata/linux/report/*
pkg/report/testdata/linux/report/40:TITLE: UBSAN: Undefined behaviour
in drivers/usb/core/devio.c:LINE
pkg/report/testdata/linux/report/40:[ 4.556972] UBSAN: Undefined
behaviour in drivers/usb/core/devio.c:1517:25
pkg/report/testdata/linux/report/41:TITLE: UBSAN: Undefined behaviour
in ./arch/x86/include/asm/atomic.h:LINE
pkg/report/testdata/linux/report/41:[ 3.805453] UBSAN: Undefined
behaviour in ./arch/x86/include/asm/atomic.h:156:2
pkg/report/testdata/linux/report/42:TITLE: UBSAN: Undefined behaviour
in kernel/time/hrtimer.c:LINE
pkg/report/testdata/linux/report/42:[ 50.583499] UBSAN: Undefined
behaviour in kernel/time/hrtimer.c:310:16
One of them is incomplete and is parsed as "corrupted kernel output"
(won't be reported):
https://github.com/google/syzkaller/blob/master/pkg/report/testdata/linux/report/42
Also I see that report parsing just takes the first line, which
includes file name, which is suboptimal (too long, can't report 2 bugs
in the same file). We seem to converge on "bug-type in function-name"
format.
The thing about bug titles is that it's harder to change them later.
If syzbot already reported 100 bugs and we change titles, it will
start re-reporting the old one after new names and the old ones will
look stale, yet they still relevant, just detected under different
name.
So we also need to get this part right before enabling.
> I noticed a few things
> about the ubsan handlers: they don't use any of the common "warn"
> infrastructure (neither does kasan from what I can see), and was missing
> a check for panic_on_warn (kasan has this, but does it incorrectly).
Yes, panic_on_warn we also need.
I will look at the patches again for Acked-by.
> I think kasan and ubsan should be reworked to use the common warn
> infrastructure, and at the very least, ubsan needs this:
>
> diff --git a/lib/ubsan.c b/lib/ubsan.c
> index e7d31735950d..a2535a62c9af 100644
> --- a/lib/ubsan.c
> +++ b/lib/ubsan.c
> @@ -160,6 +160,17 @@ static void ubsan_epilogue(unsigned long *flags)
> "========================================\n");
> spin_unlock_irqrestore(&report_lock, *flags);
> current->in_ubsan--;
> +
> + if (panic_on_warn) {
> + /*
> + * This thread may hit another WARN() in the panic path.
> + * Resetting this prevents additional WARN() from panicking the
> + * system on this thread. Other threads are blocked by the
> + * panic_mutex in panic().
> + */
> + panic_on_warn = 0;
> + panic("panic_on_warn set ...\n");
> + }
> }
>
> static void handle_overflow(struct overflow_data *data, void *lhs,
>
> > Kees, or anybody else interested, could you provide relevant configs
> > that (1) useful for kernel,
>
> As mentioned in the other email (but just to keep the note together with
> the other thoughts here) after this series, you'd want:
>
> CONFIG_UBSAN=y
> CONFIG_UBSAN_BOUNDS=y
> # CONFIG_UBSAN_MISC is not set
>
> > (2) we want 100% cleanliness,
>
> What do you mean here by "cleanliness"? It seems different from (3)
> about the test tripping a lot?
>
> > (3) don't
> > fire all the time even without fuzzing?
>
> I ran with the bounds checker enabled (and the above patch) under
> syzkaller for the weekend and saw 0 bounds checker reports.
>
> > Anything else required to
> > enable UBSAN? I don't see anything. syzbot uses gcc 8.something, which
> > I assume should be enough (but we can upgrade if necessary).
>
> As mentioned, gcc 8+ should be fine.
>
> --
> Kees Cook
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/201911262134.ED9E60965%40keescook.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-27 6:54 ` Dmitry Vyukov
@ 2019-11-27 9:34 ` Dmitry Vyukov
2019-11-27 17:59 ` Kees Cook
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2019-11-27 9:34 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
LKML, kernel-hardening, syzkaller
On Wed, Nov 27, 2019 at 7:54 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Nov 22, 2019 at 10:07:29AM +0100, Dmitry Vyukov wrote:
> > > On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > v2:
> > > > - clarify Kconfig help text (aryabinin)
> > > > - add reviewed-by
> > > > - aim series at akpm, which seems to be where ubsan goes through?
> > > > v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@chromium.org
> > > >
> > > > This splits out the bounds checker so it can be individually used. This
> > > > is expected to be enabled in Android and hopefully for syzbot. Includes
> > > > LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
> > > >
> > > > -Kees
> > >
> > > +syzkaller mailing list
> > >
> > > This is great!
> >
> > BTW, can I consider this your Acked-by for these patches? :)
> >
> > > I wanted to enable UBSAN on syzbot for a long time. And it's
> > > _probably_ not lots of work. But it was stuck on somebody actually
> > > dedicating some time specifically for it.
> >
> > Do you have a general mechanism to test that syzkaller will actually
> > pick up the kernel log splat of a new check?
>
> Yes. That's one of the most important and critical parts of syzkaller :)
> The tests for different types of bugs are here:
> https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report
>
> But have 3 for UBSAN, but they may be old and it would be useful to
> have 1 example crash per bug type:
>
> syzkaller$ grep UBSAN pkg/report/testdata/linux/report/*
> pkg/report/testdata/linux/report/40:TITLE: UBSAN: Undefined behaviour
> in drivers/usb/core/devio.c:LINE
> pkg/report/testdata/linux/report/40:[ 4.556972] UBSAN: Undefined
> behaviour in drivers/usb/core/devio.c:1517:25
> pkg/report/testdata/linux/report/41:TITLE: UBSAN: Undefined behaviour
> in ./arch/x86/include/asm/atomic.h:LINE
> pkg/report/testdata/linux/report/41:[ 3.805453] UBSAN: Undefined
> behaviour in ./arch/x86/include/asm/atomic.h:156:2
> pkg/report/testdata/linux/report/42:TITLE: UBSAN: Undefined behaviour
> in kernel/time/hrtimer.c:LINE
> pkg/report/testdata/linux/report/42:[ 50.583499] UBSAN: Undefined
> behaviour in kernel/time/hrtimer.c:310:16
>
> One of them is incomplete and is parsed as "corrupted kernel output"
> (won't be reported):
> https://github.com/google/syzkaller/blob/master/pkg/report/testdata/linux/report/42
>
> Also I see that report parsing just takes the first line, which
> includes file name, which is suboptimal (too long, can't report 2 bugs
> in the same file). We seem to converge on "bug-type in function-name"
> format.
> The thing about bug titles is that it's harder to change them later.
> If syzbot already reported 100 bugs and we change titles, it will
> start re-reporting the old one after new names and the old ones will
> look stale, yet they still relevant, just detected under different
> name.
> So we also need to get this part right before enabling.
>
> > I noticed a few things
> > about the ubsan handlers: they don't use any of the common "warn"
> > infrastructure (neither does kasan from what I can see), and was missing
> > a check for panic_on_warn (kasan has this, but does it incorrectly).
>
> Yes, panic_on_warn we also need.
>
> I will look at the patches again for Acked-by.
Acked-by: Dmitry Vyukov <dvyukov@google.com>
for the series.
I see you extended the test module, do you have samples of all UBSAN
report types that are triggered by these functions? Is so, please add
them to:
https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report
with whatever titles they are detected now. Improving titles will then
be the next step, but much simpler with a good collection of tests.
Will you send the panic_on_want patch as well?
> > I think kasan and ubsan should be reworked to use the common warn
> > infrastructure, and at the very least, ubsan needs this:
> >
> > diff --git a/lib/ubsan.c b/lib/ubsan.c
> > index e7d31735950d..a2535a62c9af 100644
> > --- a/lib/ubsan.c
> > +++ b/lib/ubsan.c
> > @@ -160,6 +160,17 @@ static void ubsan_epilogue(unsigned long *flags)
> > "========================================\n");
> > spin_unlock_irqrestore(&report_lock, *flags);
> > current->in_ubsan--;
> > +
> > + if (panic_on_warn) {
> > + /*
> > + * This thread may hit another WARN() in the panic path.
> > + * Resetting this prevents additional WARN() from panicking the
> > + * system on this thread. Other threads are blocked by the
> > + * panic_mutex in panic().
> > + */
> > + panic_on_warn = 0;
> > + panic("panic_on_warn set ...\n");
> > + }
> > }
> >
> > static void handle_overflow(struct overflow_data *data, void *lhs,
> >
> > > Kees, or anybody else interested, could you provide relevant configs
> > > that (1) useful for kernel,
> >
> > As mentioned in the other email (but just to keep the note together with
> > the other thoughts here) after this series, you'd want:
> >
> > CONFIG_UBSAN=y
> > CONFIG_UBSAN_BOUNDS=y
> > # CONFIG_UBSAN_MISC is not set
> >
> > > (2) we want 100% cleanliness,
> >
> > What do you mean here by "cleanliness"? It seems different from (3)
> > about the test tripping a lot?
> >
> > > (3) don't
> > > fire all the time even without fuzzing?
> >
> > I ran with the bounds checker enabled (and the above patch) under
> > syzkaller for the weekend and saw 0 bounds checker reports.
> >
> > > Anything else required to
> > > enable UBSAN? I don't see anything. syzbot uses gcc 8.something, which
> > > I assume should be enough (but we can upgrade if necessary).
> >
> > As mentioned, gcc 8+ should be fine.
> >
> > --
> > Kees Cook
> >
> > --
> > You received this message because you are subscribed to the Google Groups "syzkaller" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/201911262134.ED9E60965%40keescook.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-27 9:34 ` Dmitry Vyukov
@ 2019-11-27 17:59 ` Kees Cook
2019-11-28 10:38 ` Dmitry Vyukov
2019-11-28 13:10 ` Dmitry Vyukov
0 siblings, 2 replies; 15+ messages in thread
From: Kees Cook @ 2019-11-27 17:59 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
LKML, kernel-hardening, syzkaller
On Wed, Nov 27, 2019 at 10:34:24AM +0100, Dmitry Vyukov wrote:
> On Wed, Nov 27, 2019 at 7:54 AM Dmitry Vyukov <dvyukov@google.com> wrote:
> >
> > On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Fri, Nov 22, 2019 at 10:07:29AM +0100, Dmitry Vyukov wrote:
> > > > On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@chromium.org> wrote:
> > > > >
> > > > > v2:
> > > > > - clarify Kconfig help text (aryabinin)
> > > > > - add reviewed-by
> > > > > - aim series at akpm, which seems to be where ubsan goes through?
> > > > > v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@chromium.org
> > > > >
> > > > > This splits out the bounds checker so it can be individually used. This
> > > > > is expected to be enabled in Android and hopefully for syzbot. Includes
> > > > > LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
> > > > >
> > > > > -Kees
> > > >
> > > > +syzkaller mailing list
> > > >
> > > > This is great!
> > >
> > > BTW, can I consider this your Acked-by for these patches? :)
> > >
> > > > I wanted to enable UBSAN on syzbot for a long time. And it's
> > > > _probably_ not lots of work. But it was stuck on somebody actually
> > > > dedicating some time specifically for it.
> > >
> > > Do you have a general mechanism to test that syzkaller will actually
> > > pick up the kernel log splat of a new check?
> >
> > Yes. That's one of the most important and critical parts of syzkaller :)
> > The tests for different types of bugs are here:
> > https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report
> >
> > But have 3 for UBSAN, but they may be old and it would be useful to
> > have 1 example crash per bug type:
> >
> > syzkaller$ grep UBSAN pkg/report/testdata/linux/report/*
> > pkg/report/testdata/linux/report/40:TITLE: UBSAN: Undefined behaviour
> > in drivers/usb/core/devio.c:LINE
> > pkg/report/testdata/linux/report/40:[ 4.556972] UBSAN: Undefined
> > behaviour in drivers/usb/core/devio.c:1517:25
> > pkg/report/testdata/linux/report/41:TITLE: UBSAN: Undefined behaviour
> > in ./arch/x86/include/asm/atomic.h:LINE
> > pkg/report/testdata/linux/report/41:[ 3.805453] UBSAN: Undefined
> > behaviour in ./arch/x86/include/asm/atomic.h:156:2
> > pkg/report/testdata/linux/report/42:TITLE: UBSAN: Undefined behaviour
> > in kernel/time/hrtimer.c:LINE
> > pkg/report/testdata/linux/report/42:[ 50.583499] UBSAN: Undefined
> > behaviour in kernel/time/hrtimer.c:310:16
> >
> > One of them is incomplete and is parsed as "corrupted kernel output"
> > (won't be reported):
> > https://github.com/google/syzkaller/blob/master/pkg/report/testdata/linux/report/42
> >
> > Also I see that report parsing just takes the first line, which
> > includes file name, which is suboptimal (too long, can't report 2 bugs
> > in the same file). We seem to converge on "bug-type in function-name"
> > format.
> > The thing about bug titles is that it's harder to change them later.
> > If syzbot already reported 100 bugs and we change titles, it will
> > start re-reporting the old one after new names and the old ones will
> > look stale, yet they still relevant, just detected under different
> > name.
> > So we also need to get this part right before enabling.
It Sounds like instead of "UBSAN: Undefined behaviour in $file", UBSAN
should report something like "UBSAN: $behavior in $file"?
e.g.
40: UBSAN: bad shift in drivers/usb/core/devio.c:1517:25"
41: UBSAN: signed integer overflow in ./arch/x86/include/asm/atomic.h:156:2
I'll add one for the bounds checker.
How are these reports used? (And is there a way to check a live kernel
crash? i.e. to tell syzkaller "echo ARRAY_BOUNDS >/.../lkdtm..." and
generate a report?
> > > I noticed a few things
> > > about the ubsan handlers: they don't use any of the common "warn"
> > > infrastructure (neither does kasan from what I can see), and was missing
> > > a check for panic_on_warn (kasan has this, but does it incorrectly).
> >
> > Yes, panic_on_warn we also need.
> >
> > I will look at the patches again for Acked-by.
>
>
> Acked-by: Dmitry Vyukov <dvyukov@google.com>
> for the series.
Thanks!
>
> I see you extended the test module, do you have samples of all UBSAN
> report types that are triggered by these functions? Is so, please add
> them to:
> https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report
Okay, cool.
> with whatever titles they are detected now. Improving titles will then
> be the next step, but much simpler with a good collection of tests.
>
> Will you send the panic_on_want patch as well?
Yes; I wanted to make sure it was needed first (which you've confirmed
now). I'll likely not send it until next week.
--
Kees Cook
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-27 17:59 ` Kees Cook
@ 2019-11-28 10:38 ` Dmitry Vyukov
2019-11-28 16:14 ` Qian Cai
2019-11-28 13:10 ` Dmitry Vyukov
1 sibling, 1 reply; 15+ messages in thread
From: Dmitry Vyukov @ 2019-11-28 10:38 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
LKML, kernel-hardening, syzkaller
On Wed, Nov 27, 2019 at 6:59 PM Kees Cook <keescook@chromium.org> wrote:
> > > > > > v2:
> > > > > > - clarify Kconfig help text (aryabinin)
> > > > > > - add reviewed-by
> > > > > > - aim series at akpm, which seems to be where ubsan goes through?
> > > > > > v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@chromium.org
> > > > > >
> > > > > > This splits out the bounds checker so it can be individually used. This
> > > > > > is expected to be enabled in Android and hopefully for syzbot. Includes
> > > > > > LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
> > > > > >
> > > > > > -Kees
> > > > >
> > > > > +syzkaller mailing list
> > > > >
> > > > > This is great!
> > > >
> > > > BTW, can I consider this your Acked-by for these patches? :)
> > > >
> > > > > I wanted to enable UBSAN on syzbot for a long time. And it's
> > > > > _probably_ not lots of work. But it was stuck on somebody actually
> > > > > dedicating some time specifically for it.
> > > >
> > > > Do you have a general mechanism to test that syzkaller will actually
> > > > pick up the kernel log splat of a new check?
> > >
> > > Yes. That's one of the most important and critical parts of syzkaller :)
> > > The tests for different types of bugs are here:
> > > https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report
> > >
> > > But have 3 for UBSAN, but they may be old and it would be useful to
> > > have 1 example crash per bug type:
> > >
> > > syzkaller$ grep UBSAN pkg/report/testdata/linux/report/*
> > > pkg/report/testdata/linux/report/40:TITLE: UBSAN: Undefined behaviour
> > > in drivers/usb/core/devio.c:LINE
> > > pkg/report/testdata/linux/report/40:[ 4.556972] UBSAN: Undefined
> > > behaviour in drivers/usb/core/devio.c:1517:25
> > > pkg/report/testdata/linux/report/41:TITLE: UBSAN: Undefined behaviour
> > > in ./arch/x86/include/asm/atomic.h:LINE
> > > pkg/report/testdata/linux/report/41:[ 3.805453] UBSAN: Undefined
> > > behaviour in ./arch/x86/include/asm/atomic.h:156:2
> > > pkg/report/testdata/linux/report/42:TITLE: UBSAN: Undefined behaviour
> > > in kernel/time/hrtimer.c:LINE
> > > pkg/report/testdata/linux/report/42:[ 50.583499] UBSAN: Undefined
> > > behaviour in kernel/time/hrtimer.c:310:16
> > >
> > > One of them is incomplete and is parsed as "corrupted kernel output"
> > > (won't be reported):
> > > https://github.com/google/syzkaller/blob/master/pkg/report/testdata/linux/report/42
> > >
> > > Also I see that report parsing just takes the first line, which
> > > includes file name, which is suboptimal (too long, can't report 2 bugs
> > > in the same file). We seem to converge on "bug-type in function-name"
> > > format.
> > > The thing about bug titles is that it's harder to change them later.
> > > If syzbot already reported 100 bugs and we change titles, it will
> > > start re-reporting the old one after new names and the old ones will
> > > look stale, yet they still relevant, just detected under different
> > > name.
> > > So we also need to get this part right before enabling.
>
> It Sounds like instead of "UBSAN: Undefined behaviour in $file", UBSAN
> should report something like "UBSAN: $behavior in $file"?
>
> e.g.
> 40: UBSAN: bad shift in drivers/usb/core/devio.c:1517:25"
> 41: UBSAN: signed integer overflow in ./arch/x86/include/asm/atomic.h:156:2
If you mean make them such that kernel testing systems could simply
take the first line as "crash identity", then most likely we need
function name there instead of file:line:column. At least this seems
to be working the best based on our experience.
> I'll add one for the bounds checker.
>
> How are these reports used?
There are test inputs, each also contains expected parsing output
(title at minimum, but can also contain crash type, corrupted mark,
extracted "report") and that's verified against actual parsing result.
> (And is there a way to check a live kernel
> crash? i.e. to tell syzkaller "echo ARRAY_BOUNDS >/.../lkdtm..." and
> generate a report?
Unfortunately all of kernel tooling is completely untested at the
moment. We would very much like to have all sanitizers tested in a
meaningful way, e.g.:
https://github.com/llvm-mirror/compiler-rt/blob/master/test/asan/TestCases/global-overflow.cpp#L15-L18
But also LOCKDEP, KMEMLEAK, ODEBUG, FAULT_INJECTS, etc, all untested
too. Nobody knows what they produce, and if they even still detect
bugs, report false positives, etc.
But that's the kernel testing story...
No, syzbot does not do kernels unit-testing. And there are no such
tests anyways...
> > > > I noticed a few things
> > > > about the ubsan handlers: they don't use any of the common "warn"
> > > > infrastructure (neither does kasan from what I can see), and was missing
> > > > a check for panic_on_warn (kasan has this, but does it incorrectly).
> > >
> > > Yes, panic_on_warn we also need.
> > >
> > > I will look at the patches again for Acked-by.
> >
> >
> > Acked-by: Dmitry Vyukov <dvyukov@google.com>
> > for the series.
>
> Thanks!
>
> >
> > I see you extended the test module, do you have samples of all UBSAN
> > report types that are triggered by these functions? Is so, please add
> > them to:
> > https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report
>
> Okay, cool.
>
> > with whatever titles they are detected now. Improving titles will then
> > be the next step, but much simpler with a good collection of tests.
> >
> > Will you send the panic_on_want patch as well?
>
> Yes; I wanted to make sure it was needed first (which you've confirmed
> now). I'll likely not send it until next week.
>
> --
> Kees Cook
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller/201911270952.D66CD15AEC%40keescook.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-28 10:38 ` Dmitry Vyukov
@ 2019-11-28 16:14 ` Qian Cai
0 siblings, 0 replies; 15+ messages in thread
From: Qian Cai @ 2019-11-28 16:14 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Kees Cook, Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Arnd Bergmann, Ard Biesheuvel, kasan-dev,
LKML, kernel-hardening, syzkaller
> On Nov 28, 2019, at 5:39 AM, 'Dmitry Vyukov' via kasan-dev <kasan-dev@googlegroups.com> wrote:
>
> But also LOCKDEP, KMEMLEAK, ODEBUG, FAULT_INJECTS, etc, all untested
> too. Nobody knows what they produce, and if they even still detect
> bugs, report false positives, etc.
> But that's the kernel testing story...
Yes, those work except PROVE_LOCKING where there are existing potential deadlocks are almost impossible to fix them properly now. I have been running those for linux-next daily with all those debugging on where you can borrow the configs etc.
https://github.com/cailca/linux-mm
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 0/3] ubsan: Split out bounds checker
2019-11-27 17:59 ` Kees Cook
2019-11-28 10:38 ` Dmitry Vyukov
@ 2019-11-28 13:10 ` Dmitry Vyukov
1 sibling, 0 replies; 15+ messages in thread
From: Dmitry Vyukov @ 2019-11-28 13:10 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Andrey Ryabinin, Elena Petrova,
Alexander Potapenko, Linus Torvalds, Dan Carpenter,
Gustavo A. R. Silva, Ard Biesheuvel, kasan-dev, LKML,
kernel-hardening, syzkaller
On Wed, Nov 27, 2019 at 6:59 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Nov 27, 2019 at 10:34:24AM +0100, Dmitry Vyukov wrote:
> > On Wed, Nov 27, 2019 at 7:54 AM Dmitry Vyukov <dvyukov@google.com> wrote:
> > >
> > > On Wed, Nov 27, 2019 at 6:42 AM Kees Cook <keescook@chromium.org> wrote:
> > > >
> > > > On Fri, Nov 22, 2019 at 10:07:29AM +0100, Dmitry Vyukov wrote:
> > > > > On Thu, Nov 21, 2019 at 7:15 PM Kees Cook <keescook@chromium.org> wrote:
> > > > > >
> > > > > > v2:
> > > > > > - clarify Kconfig help text (aryabinin)
> > > > > > - add reviewed-by
> > > > > > - aim series at akpm, which seems to be where ubsan goes through?
> > > > > > v1: https://lore.kernel.org/lkml/20191120010636.27368-1-keescook@chromium.org
> > > > > >
> > > > > > This splits out the bounds checker so it can be individually used. This
> > > > > > is expected to be enabled in Android and hopefully for syzbot. Includes
> > > > > > LKDTM tests for behavioral corner-cases (beyond just the bounds checker).
> > > > > >
> > > > > > -Kees
> > > > >
> > > > > +syzkaller mailing list
> > > > >
> > > > > This is great!
> > > >
> > > > BTW, can I consider this your Acked-by for these patches? :)
> > > >
> > > > > I wanted to enable UBSAN on syzbot for a long time. And it's
> > > > > _probably_ not lots of work. But it was stuck on somebody actually
> > > > > dedicating some time specifically for it.
> > > >
> > > > Do you have a general mechanism to test that syzkaller will actually
> > > > pick up the kernel log splat of a new check?
> > >
> > > Yes. That's one of the most important and critical parts of syzkaller :)
> > > The tests for different types of bugs are here:
> > > https://github.com/google/syzkaller/tree/master/pkg/report/testdata/linux/report
> > >
> > > But have 3 for UBSAN, but they may be old and it would be useful to
> > > have 1 example crash per bug type:
> > >
> > > syzkaller$ grep UBSAN pkg/report/testdata/linux/report/*
> > > pkg/report/testdata/linux/report/40:TITLE: UBSAN: Undefined behaviour
> > > in drivers/usb/core/devio.c:LINE
> > > pkg/report/testdata/linux/report/40:[ 4.556972] UBSAN: Undefined
> > > behaviour in drivers/usb/core/devio.c:1517:25
> > > pkg/report/testdata/linux/report/41:TITLE: UBSAN: Undefined behaviour
> > > in ./arch/x86/include/asm/atomic.h:LINE
> > > pkg/report/testdata/linux/report/41:[ 3.805453] UBSAN: Undefined
> > > behaviour in ./arch/x86/include/asm/atomic.h:156:2
> > > pkg/report/testdata/linux/report/42:TITLE: UBSAN: Undefined behaviour
> > > in kernel/time/hrtimer.c:LINE
> > > pkg/report/testdata/linux/report/42:[ 50.583499] UBSAN: Undefined
> > > behaviour in kernel/time/hrtimer.c:310:16
> > >
> > > One of them is incomplete and is parsed as "corrupted kernel output"
> > > (won't be reported):
> > > https://github.com/google/syzkaller/blob/master/pkg/report/testdata/linux/report/42
> > >
> > > Also I see that report parsing just takes the first line, which
> > > includes file name, which is suboptimal (too long, can't report 2 bugs
> > > in the same file). We seem to converge on "bug-type in function-name"
> > > format.
> > > The thing about bug titles is that it's harder to change them later.
> > > If syzbot already reported 100 bugs and we change titles, it will
> > > start re-reporting the old one after new names and the old ones will
> > > look stale, yet they still relevant, just detected under different
> > > name.
> > > So we also need to get this part right before enabling.
>
> It Sounds like instead of "UBSAN: Undefined behaviour in $file", UBSAN
> should report something like "UBSAN: $behavior in $file"?
>
> e.g.
> 40: UBSAN: bad shift in drivers/usb/core/devio.c:1517:25"
> 41: UBSAN: signed integer overflow in ./arch/x86/include/asm/atomic.h:156:2
>
> I'll add one for the bounds checker.
>
> How are these reports used? (And is there a way to check a live kernel
> crash? i.e. to tell syzkaller "echo ARRAY_BOUNDS >/.../lkdtm..." and
> generate a report?
I've collected the sample and added to syzkaller test base:
https://github.com/google/syzkaller/commit/76357d6f894431c00cc09cfc9e7474701a4b822a
I also filed https://github.com/google/syzkaller/issues/1523 for
enabling UBSAN on syzbot, let's move syzbot-related discussion there.
^ permalink raw reply [flat|nested] 15+ messages in thread