[PATCH] kconfig: Add kernel config option for fuzz testing.

[PATCH] kconfig: Add kernel config option for fuzz testing.

[Tetsuo Handa]

While syzkaller is finding many bugs, sometimes syzkaller examines
stupid operations. But disabling operations using kernel config option
is problematic because "kernel config option excludes whole module when
there is still room for examining all but specific operation" and
"the list of kernel config options becomes too complicated to maintain
because such list changes over time". Thus, 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.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
 drivers/char/mem.c                  |  6 +++---
 drivers/tty/serial/8250/8250_port.c |  7 +++++--
 drivers/tty/vt/keyboard.c           |  3 +++
 fs/ioctl.c                          |  5 +++++
 include/linux/uaccess.h             | 12 ++++++++++++
 kernel/printk/printk.c              |  8 ++++++++
 lib/Kconfig.debug                   | 10 ++++++++++
 lib/usercopy.c                      | 27 +++++++++++++++++++++++++++
 8 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 43dd0891ca1e..63aff3ae7c2b 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -246,7 +246,7 @@ static ssize_t write_mem(struct file *file, const char __user *buf,
 				return -EFAULT;
 			}
 
-			copied = copy_from_user(ptr, buf, sz);
+			copied = copy_from_user_to_any(ptr, buf, sz);
 			unxlate_dev_mem_ptr(p, ptr);
 			if (copied) {
 				written += sz - copied;
@@ -550,7 +550,7 @@ static ssize_t do_write_kmem(unsigned long p, const char __user *buf,
 		if (!virt_addr_valid(ptr))
 			return -ENXIO;
 
-		copied = copy_from_user(ptr, buf, sz);
+		copied = copy_from_user_to_any(ptr, buf, sz);
 		if (copied) {
 			written += sz - copied;
 			if (written)
@@ -604,7 +604,7 @@ static ssize_t write_kmem(struct file *file, const char __user *buf,
 				err = -ENXIO;
 				break;
 			}
-			n = copy_from_user(kbuf, buf, sz);
+			n = copy_from_user_to_any(kbuf, buf, sz);
 			if (n) {
 				err = -EFAULT;
 				break;
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 90655910b0c7..367b92ad598b 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -519,11 +519,14 @@ serial_port_out_sync(struct uart_port *p, int offset, int value)
 	case UPIO_MEM32:
 	case UPIO_MEM32BE:
 	case UPIO_AU:
-		p->serial_out(p, offset, value);
+		/* Writing to random kernel address causes crash. */
+		if (!IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
+			p->serial_out(p, offset, value);
 		p->serial_in(p, UART_LCR);	/* safe, no side-effects */
 		break;
 	default:
-		p->serial_out(p, offset, value);
+		if (!IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
+			p->serial_out(p, offset, value);
 	}
 }
 
diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
index 15d33fa0c925..367820b8ff59 100644
--- a/drivers/tty/vt/keyboard.c
+++ b/drivers/tty/vt/keyboard.c
@@ -633,6 +633,9 @@ static void k_spec(struct vc_data *vc, unsigned char value, char up_flag)
 	     kbd->kbdmode == VC_OFF) &&
 	     value != KVAL(K_SAK))
 		return;		/* SAK is allowed even in raw mode */
+	/* Repeating SysRq-t forever causes RCU stalls. */
+	if (IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
+		return;
 	fn_handler[value](vc);
 }
 
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 2f5e4e5b97e1..f879aa94b118 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -601,6 +601,11 @@ static int ioctl_fsfreeze(struct file *filp)
 	if (sb->s_op->freeze_fs == NULL && sb->s_op->freeze_super == NULL)
 		return -EOPNOTSUPP;
 
+#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
+	/* Freezing filesystems causes hung tasks. */
+	return -EBUSY;
+#endif
+
 	/* Freeze */
 	if (sb->s_op->freeze_super)
 		return sb->s_op->freeze_super(sb);
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 67f016010aad..6e5ddd0fdcce 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -387,4 +387,16 @@ void __noreturn usercopy_abort(const char *name, const char *detail,
 			       unsigned long len);
 #endif
 
+#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
+unsigned long __must_check copy_from_user_to_any(void *to,
+						 const void __user *from,
+						 unsigned long n);
+#else
+static __always_inline unsigned long __must_check
+copy_from_user_to_any(void *to, const void __user *from, unsigned long n)
+{
+	return copy_from_user(to, from, n);
+}
+#endif
+
 #endif		/* __LINUX_UACCESS_H__ */
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1ef6f75d92f1..9a2f95a78fef 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1198,6 +1198,14 @@ MODULE_PARM_DESC(ignore_loglevel,
 
 static bool suppress_message_printing(int level)
 {
+#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
+	/*
+	 * Changing console_loglevel causes "no output". But ignoring
+	 * console_loglevel is easier than preventing change of
+	 * console_loglevel.
+	 */
+	return (level >= CONSOLE_LOGLEVEL_DEFAULT && !ignore_loglevel);
+#endif
 	return (level >= console_loglevel && !ignore_loglevel);
 }
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index d1842fe756d5..f9836cc23942 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2184,4 +2184,14 @@ config HYPERV_TESTING
 	help
 	  Select this option to enable Hyper-V vmbus testing.
 
+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
diff --git a/lib/usercopy.c b/lib/usercopy.c
index cbb4d9ec00f2..f7d5d243a63d 100644
--- a/lib/usercopy.c
+++ b/lib/usercopy.c
@@ -86,3 +86,30 @@ int check_zeroed_user(const void __user *from, size_t size)
 	return -EFAULT;
 }
 EXPORT_SYMBOL(check_zeroed_user);
+
+#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
+/*
+ * Since copying to arbitrary user-specified kernel address will crash
+ * the kernel trivially, do not copy to user-specified kernel address
+ * if the kernel was build for fuzz testing.
+ */
+unsigned long __must_check copy_from_user_to_any(void *to,
+						 const void __user *from,
+						 unsigned long n)
+{
+	static char dummybuf[PAGE_SIZE];
+	unsigned long ret = 0;
+
+	while (n) {
+		unsigned long size = min(n, sizeof(dummybuf));
+
+		ret = copy_from_user(dummybuf, from, size);
+		if (ret)
+			break;
+		from += size;
+		n -= size;
+	}
+	return ret;
+}
+EXPORT_SYMBOL(copy_from_user_to_any);
+#endif
-- 
2.18.1

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Greg Kroah-Hartman]

On Mon, Dec 16, 2019 at 06:59:55PM +0900, Tetsuo Handa wrote:
> While syzkaller is finding many bugs, sometimes syzkaller examines
> stupid operations. But disabling operations using kernel config option
> is problematic because "kernel config option excludes whole module when
> there is still room for examining all but specific operation" and
> "the list of kernel config options becomes too complicated to maintain
> because such list changes over time". Thus, 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.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> ---
>  drivers/char/mem.c                  |  6 +++---
>  drivers/tty/serial/8250/8250_port.c |  7 +++++--
>  drivers/tty/vt/keyboard.c           |  3 +++
>  fs/ioctl.c                          |  5 +++++
>  include/linux/uaccess.h             | 12 ++++++++++++
>  kernel/printk/printk.c              |  8 ++++++++
>  lib/Kconfig.debug                   | 10 ++++++++++
>  lib/usercopy.c                      | 27 +++++++++++++++++++++++++++
>  8 files changed, 73 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 43dd0891ca1e..63aff3ae7c2b 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -246,7 +246,7 @@ static ssize_t write_mem(struct file *file, const char __user *buf,
>  				return -EFAULT;
>  			}
>  
> -			copied = copy_from_user(ptr, buf, sz);
> +			copied = copy_from_user_to_any(ptr, buf, sz);

That name gives no context at all what is happening here.

And for the record, I really do not like this.

Why isn't it the job of the fuzzers to keep a blacklist of things they
should not do without expecting the system to crash?  Why is it up to
the kernel to do that work for them?


>  			unxlate_dev_mem_ptr(p, ptr);
>  			if (copied) {
>  				written += sz - copied;
> @@ -550,7 +550,7 @@ static ssize_t do_write_kmem(unsigned long p, const char __user *buf,
>  		if (!virt_addr_valid(ptr))
>  			return -ENXIO;
>  
> -		copied = copy_from_user(ptr, buf, sz);
> +		copied = copy_from_user_to_any(ptr, buf, sz);
>  		if (copied) {
>  			written += sz - copied;
>  			if (written)
> @@ -604,7 +604,7 @@ static ssize_t write_kmem(struct file *file, const char __user *buf,
>  				err = -ENXIO;
>  				break;
>  			}
> -			n = copy_from_user(kbuf, buf, sz);
> +			n = copy_from_user_to_any(kbuf, buf, sz);
>  			if (n) {
>  				err = -EFAULT;
>  				break;
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 90655910b0c7..367b92ad598b 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -519,11 +519,14 @@ serial_port_out_sync(struct uart_port *p, int offset, int value)
>  	case UPIO_MEM32:
>  	case UPIO_MEM32BE:
>  	case UPIO_AU:
> -		p->serial_out(p, offset, value);
> +		/* Writing to random kernel address causes crash. */
> +		if (!IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
> +			p->serial_out(p, offset, value);
>  		p->serial_in(p, UART_LCR);	/* safe, no side-effects */
>  		break;
>  	default:
> -		p->serial_out(p, offset, value);
> +		if (!IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
> +			p->serial_out(p, offset, value);
>  	}
>  }
>  
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 15d33fa0c925..367820b8ff59 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -633,6 +633,9 @@ static void k_spec(struct vc_data *vc, unsigned char value, char up_flag)
>  	     kbd->kbdmode == VC_OFF) &&
>  	     value != KVAL(K_SAK))
>  		return;		/* SAK is allowed even in raw mode */
> +	/* Repeating SysRq-t forever causes RCU stalls. */
> +	if (IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
> +		return;

That does not make sense.  What does this comment mean?



>  	fn_handler[value](vc);
>  }
>  
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 2f5e4e5b97e1..f879aa94b118 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -601,6 +601,11 @@ static int ioctl_fsfreeze(struct file *filp)
>  	if (sb->s_op->freeze_fs == NULL && sb->s_op->freeze_super == NULL)
>  		return -EOPNOTSUPP;
>  
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +	/* Freezing filesystems causes hung tasks. */
> +	return -EBUSY;
> +#endif

ick ick ick.

> +
>  	/* Freeze */
>  	if (sb->s_op->freeze_super)
>  		return sb->s_op->freeze_super(sb);
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 67f016010aad..6e5ddd0fdcce 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -387,4 +387,16 @@ void __noreturn usercopy_abort(const char *name, const char *detail,
>  			       unsigned long len);
>  #endif
>  
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +unsigned long __must_check copy_from_user_to_any(void *to,
> +						 const void __user *from,
> +						 unsigned long n);
> +#else
> +static __always_inline unsigned long __must_check
> +copy_from_user_to_any(void *to, const void __user *from, unsigned long n)

Again, this name does not make any sense to me.
You provide no documentation for when this should, or should not, be
used instead of the "normal" copy_from_user() call.

Again, not that I think this is ok at all.

> +{
> +	return copy_from_user(to, from, n);
> +}
> +#endif
> +
>  #endif		/* __LINUX_UACCESS_H__ */
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 1ef6f75d92f1..9a2f95a78fef 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1198,6 +1198,14 @@ MODULE_PARM_DESC(ignore_loglevel,
>  
>  static bool suppress_message_printing(int level)
>  {
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +	/*
> +	 * Changing console_loglevel causes "no output". But ignoring
> +	 * console_loglevel is easier than preventing change of
> +	 * console_loglevel.
> +	 */
> +	return (level >= CONSOLE_LOGLEVEL_DEFAULT && !ignore_loglevel);
> +#endif

I don't understand the need for this change at all.

>  	return (level >= console_loglevel && !ignore_loglevel);
>  }
>  
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index d1842fe756d5..f9836cc23942 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2184,4 +2184,14 @@ config HYPERV_TESTING
>  	help
>  	  Select this option to enable Hyper-V vmbus testing.
>  
> +config KERNEL_BUILT_FOR_FUZZ_TESTING
> +       bool "Build kernel for fuzz testing"
> +       default n

That is always the default, no need to list it.

> +       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
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index cbb4d9ec00f2..f7d5d243a63d 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -86,3 +86,30 @@ int check_zeroed_user(const void __user *from, size_t size)
>  	return -EFAULT;
>  }
>  EXPORT_SYMBOL(check_zeroed_user);
> +
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +/*
> + * Since copying to arbitrary user-specified kernel address will crash
> + * the kernel trivially, do not copy to user-specified kernel address
> + * if the kernel was build for fuzz testing.
> + */
> +unsigned long __must_check copy_from_user_to_any(void *to,
> +						 const void __user *from,
> +						 unsigned long n)
> +{
> +	static char dummybuf[PAGE_SIZE];
> +	unsigned long ret = 0;
> +
> +	while (n) {
> +		unsigned long size = min(n, sizeof(dummybuf));
> +
> +		ret = copy_from_user(dummybuf, from, size);
> +		if (ret)
> +			break;
> +		from += size;
> +		n -= size;

Why???  What does this "exercise" any different from a stubbed out call
to copy_from_user()?

greg k-h

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Tetsuo Handa]

On 2019/12/16 20:46, Greg Kroah-Hartman wrote:
> Why isn't it the job of the fuzzers to keep a blacklist of things they
> should not do without expecting the system to crash?  Why is it up to
> the kernel to do that work for them?

Details will be explained by Dmitry Vyukov.
In short, fuzzer wants cooperation with the kernel.

>> @@ -633,6 +633,9 @@ static void k_spec(struct vc_data *vc, unsigned char value, char up_flag)
>>  	     kbd->kbdmode == VC_OFF) &&
>>  	     value != KVAL(K_SAK))
>>  		return;		/* SAK is allowed even in raw mode */
>> +	/* Repeating SysRq-t forever causes RCU stalls. */
>> +	if (IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
>> +		return;
> 
> That does not make sense.  What does this comment mean?

https://syzkaller.appspot.com/text?tag=CrashLog&x=12eb1e67600000 (already gone)
reported that a USB fuzz test is triggering SysRq-t until RCU stall happens. We don't
need to disable whole module only for avoiding RCU stall due to crazy SysRq-t requests.

[  563.439044][    C1] ksoftirqd/1     R  running task    28264    16      2 0x80004008
[  563.447037][    C1] Call Trace:
[  563.450321][    C1]  sched_show_task.cold+0x2e0/0x359
[  563.455502][    C1]  show_state_filter+0x164/0x209
[  563.460421][    C1]  ? fn_caps_on+0x90/0x90
[  563.464730][    C1]  k_spec+0xdc/0x120
[  563.468605][    C1]  kbd_event+0x927/0x3790
[  563.472914][    C1]  ? k_pad+0x720/0x720
[  563.476964][    C1]  ? mark_held_locks+0xe0/0xe0
[  563.481791][    C1]  ? sysrq_filter+0xdf/0xeb0
[  563.486359][    C1]  ? k_pad+0x720/0x720
[  563.490419][    C1]  input_to_handler+0x3b6/0x4c0
[  563.495247][    C1]  input_pass_values.part.0+0x2e3/0x720
[  563.500770][    C1]  input_repeat_key+0x1ee/0x2c0
[  563.505622][    C1]  ? input_dev_suspend+0x80/0x80
[  563.510535][    C1]  ? rcu_read_lock_bh_held+0xb0/0xb0
[  563.515805][    C1]  call_timer_fn+0x179/0x650
[  563.520385][    C1]  ? input_dev_suspend+0x80/0x80
[  563.525389][    C1]  ? msleep_interruptible+0x130/0x130
[  563.531528][    C1]  ? rcu_read_lock_sched_held+0x9c/0xd0
[  563.537058][    C1]  ? rcu_read_lock_bh_held+0xb0/0xb0
[  563.542334][    C1]  ? _raw_spin_unlock_irq+0x24/0x30
[  563.547519][    C1]  ? input_dev_suspend+0x80/0x80
[  563.552443][    C1]  run_timer_softirq+0x5e3/0x1490
[  563.557454][    C1]  ? add_timer+0x7a0/0x7a0
[  563.561853][    C1]  ? rcu_read_lock_sched_held+0x9c/0xd0
[  563.567377][    C1]  ? rcu_read_lock_bh_held+0xb0/0xb0
[  563.572644][    C1]  __do_softirq+0x221/0x912
[  563.577147][    C1]  ? takeover_tasklets+0x720/0x720
[  563.582246][    C1]  run_ksoftirqd+0x1f/0x40
[  563.586638][    C1]  smpboot_thread_fn+0x3e8/0x850
[  563.591640][    C1]  ? smpboot_unregister_percpu_thread+0x190/0x190
[  563.598031][    C1]  ? __kthread_parkme+0x10a/0x1c0
[  563.603031][    C1]  ? smpboot_unregister_percpu_thread+0x190/0x190
[  563.609440][    C1]  kthread+0x318/0x420
[  563.613746][    C1]  ? kthread_create_on_node+0xf0/0xf0
[  563.619092][    C1]  ret_from_fork+0x24/0x30



>> diff --git a/fs/ioctl.c b/fs/ioctl.c
>> index 2f5e4e5b97e1..f879aa94b118 100644
>> --- a/fs/ioctl.c
>> +++ b/fs/ioctl.c
>> @@ -601,6 +601,11 @@ static int ioctl_fsfreeze(struct file *filp)
>>  	if (sb->s_op->freeze_fs == NULL && sb->s_op->freeze_super == NULL)
>>  		return -EOPNOTSUPP;
>>  
>> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
>> +	/* Freezing filesystems causes hung tasks. */
>> +	return -EBUSY;
>> +#endif
> 
> ick ick ick.

syzbot was failing to blacklist ioctl(FIFREEZE) due to a bug fixed at
https://lore.kernel.org/linux-fsdevel/03633af8-fab8-a985-c215-f619ea4ded08@I-love.SAKURA.ne.jp/ .

Then, syzbot was not blacklisting ioctl(EXT4_IOC_SHUTDOWN) which was fixed at
https://github.com/google/syzkaller/commit/61ed43a86a3721708aeeee72b23bfa1eacd921b2 .

These bugs could be fixed (in a whack-a-mole manner) but

>> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
>> index 1ef6f75d92f1..9a2f95a78fef 100644
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -1198,6 +1198,14 @@ MODULE_PARM_DESC(ignore_loglevel,
>>  
>>  static bool suppress_message_printing(int level)
>>  {
>> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
>> +	/*
>> +	 * Changing console_loglevel causes "no output". But ignoring
>> +	 * console_loglevel is easier than preventing change of
>> +	 * console_loglevel.
>> +	 */
>> +	return (level >= CONSOLE_LOGLEVEL_DEFAULT && !ignore_loglevel);
>> +#endif
> 
> I don't understand the need for this change at all.

this case was too hard to blacklist, as explained at
https://lore.kernel.org/lkml/4d1a4b51-999b-63c6-5ce3-a704013cecb6@i-love.sakura.ne.jp/ .
syz_execute_func() can find deeper bug by executing arbitrary binary code, but
we cannot blacklist specific syscalls/arguments for syz_execute_func() testcases.
Unless we guard on the kernel side, we won't be able to re-enable syz_execute_func()
testcases.

> 
>>  	return (level >= console_loglevel && !ignore_loglevel);
>>  }
>>  

Also, although ability to interpret kernel messages was improved by CONFIG_PRINTK_CALLER
config option, we still cannot tell whether some string such as "BUG:" "WARNING:" came from
BUG(), BUG_ON(), WARN(), WARN_ON() etc. Therefore, modules emitting such string causes
syzbot to report as crash. For example, TOMOYO security module introduced
CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING in order to allow syzbot to fuzz TOMOYO
security module, and then extended CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING to
prevent TOMOYO from emitting "WARNING:" string so that syzbot will not detect it as crash.


commit e80b18599a39a625bc8b2e39ba3004a62f78805a
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date:   Fri Apr 12 20:04:54 2019 +0900

    tomoyo: Add a kernel config option for fuzzing testing.

    syzbot is reporting kernel panic triggered by memory allocation fault
    injection before loading TOMOYO's policy [1]. To make the fuzzing tests
    useful, we need to assign a profile other than "disabled" (no-op) mode.
    Therefore, let's allow syzbot to load TOMOYO's built-in policy for
    "learning" mode using a kernel config option. This option must not be
    enabled for kernels built for production system, for this option also
    disables domain/program checks when modifying policy configuration via
    /sys/kernel/security/tomoyo/ interface.

    [1] https://syzkaller.appspot.com/bug?extid=29569ed06425fcf67a95

    Reported-by: syzbot <syzbot+e1b8084e532b6ee7afab@syzkaller.appspotmail.com>
    Reported-by: syzbot <syzbot+29569ed06425fcf67a95@syzkaller.appspotmail.com>
    Reported-by: syzbot <syzbot+2ee3f8974c2e7dc69feb@syzkaller.appspotmail.com>
    Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
    Signed-off-by: James Morris <jamorris@linux.microsoft.com>

commit 4ad98ac46490d5f8441025930070eaf028cfd0f2
Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Date:   Tue May 7 20:34:22 2019 +0900

    tomoyo: Don't emit WARNING: string while fuzzing testing.

    Commit cff0e6c3ec3e6230 ("tomoyo: Add a kernel config option for fuzzing
    testing.") enabled the learning mode, but syzkaller is detecting any
    "WARNING:" string as a crash. Thus, disable TOMOYO's quota warning if
    built for fuzzing testing.

    Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
    Cc: Dmitry Vyukov <dvyukov@google.com>
    Signed-off-by: James Morris <jamorris@linux.microsoft.com>


As a result of these two patches, TOMOYO is now fuzzed by syzbot. But there are
other modules emitting these strings. We need to somehow make it possible to
distinguish whether these string came from BUG(), BUG_ON(), WARN(), WARN_ON() etc.
in a generically applicable way.



To summarize, syzbot wants a kernel config option for two purposes:

  (1) Allow syzkaller run testcases as far/deep as possible.

  (2) Prevent syzkaller from generating false alarms.

This is beyond merely maintaining blacklist on the fuzzer side.

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Greg Kroah-Hartman]

On Tue, Dec 17, 2019 at 12:35:00AM +0900, Tetsuo Handa wrote:
> On 2019/12/16 20:46, Greg Kroah-Hartman wrote:
> > Why isn't it the job of the fuzzers to keep a blacklist of things they
> > should not do without expecting the system to crash?  Why is it up to
> > the kernel to do that work for them?
> 
> Details will be explained by Dmitry Vyukov.

Details should have been in the commit message :(

> In short, fuzzer wants cooperation with the kernel.

To a point, that's fine.  Making odd changes that are not
self-documenting or understandable even in the commit that is trying to
tadd them, is not ok at all.

> >> @@ -633,6 +633,9 @@ static void k_spec(struct vc_data *vc, unsigned char value, char up_flag)
> >>  	     kbd->kbdmode == VC_OFF) &&
> >>  	     value != KVAL(K_SAK))
> >>  		return;		/* SAK is allowed even in raw mode */
> >> +	/* Repeating SysRq-t forever causes RCU stalls. */
> >> +	if (IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
> >> +		return;
> > 
> > That does not make sense.  What does this comment mean?
> 
> https://syzkaller.appspot.com/text?tag=CrashLog&x=12eb1e67600000 (already gone)
> reported that a USB fuzz test is triggering SysRq-t until RCU stall happens. We don't
> need to disable whole module only for avoiding RCU stall due to crazy SysRq-t requests.
> 
> [  563.439044][    C1] ksoftirqd/1     R  running task    28264    16      2 0x80004008
> [  563.447037][    C1] Call Trace:
> [  563.450321][    C1]  sched_show_task.cold+0x2e0/0x359
> [  563.455502][    C1]  show_state_filter+0x164/0x209
> [  563.460421][    C1]  ? fn_caps_on+0x90/0x90
> [  563.464730][    C1]  k_spec+0xdc/0x120
> [  563.468605][    C1]  kbd_event+0x927/0x3790
> [  563.472914][    C1]  ? k_pad+0x720/0x720
> [  563.476964][    C1]  ? mark_held_locks+0xe0/0xe0
> [  563.481791][    C1]  ? sysrq_filter+0xdf/0xeb0
> [  563.486359][    C1]  ? k_pad+0x720/0x720
> [  563.490419][    C1]  input_to_handler+0x3b6/0x4c0
> [  563.495247][    C1]  input_pass_values.part.0+0x2e3/0x720
> [  563.500770][    C1]  input_repeat_key+0x1ee/0x2c0
> [  563.505622][    C1]  ? input_dev_suspend+0x80/0x80
> [  563.510535][    C1]  ? rcu_read_lock_bh_held+0xb0/0xb0
> [  563.515805][    C1]  call_timer_fn+0x179/0x650
> [  563.520385][    C1]  ? input_dev_suspend+0x80/0x80
> [  563.525389][    C1]  ? msleep_interruptible+0x130/0x130
> [  563.531528][    C1]  ? rcu_read_lock_sched_held+0x9c/0xd0
> [  563.537058][    C1]  ? rcu_read_lock_bh_held+0xb0/0xb0
> [  563.542334][    C1]  ? _raw_spin_unlock_irq+0x24/0x30
> [  563.547519][    C1]  ? input_dev_suspend+0x80/0x80
> [  563.552443][    C1]  run_timer_softirq+0x5e3/0x1490
> [  563.557454][    C1]  ? add_timer+0x7a0/0x7a0
> [  563.561853][    C1]  ? rcu_read_lock_sched_held+0x9c/0xd0
> [  563.567377][    C1]  ? rcu_read_lock_bh_held+0xb0/0xb0
> [  563.572644][    C1]  __do_softirq+0x221/0x912
> [  563.577147][    C1]  ? takeover_tasklets+0x720/0x720
> [  563.582246][    C1]  run_ksoftirqd+0x1f/0x40
> [  563.586638][    C1]  smpboot_thread_fn+0x3e8/0x850
> [  563.591640][    C1]  ? smpboot_unregister_percpu_thread+0x190/0x190
> [  563.598031][    C1]  ? __kthread_parkme+0x10a/0x1c0
> [  563.603031][    C1]  ? smpboot_unregister_percpu_thread+0x190/0x190
> [  563.609440][    C1]  kthread+0x318/0x420
> [  563.613746][    C1]  ? kthread_create_on_node+0xf0/0xf0
> [  563.619092][    C1]  ret_from_fork+0x24/0x30

Don't do sysrq-t then :)

And as I said above, make these totally and completly obvious what you
are doing.  At worst, this should have been a patch series that gives
this type of information in each and every place it is added, so at
least it is documented somewhere.  At best, it needs to be in the code
itself to make it obvious what you are doing and why it needs to stay
there for all eternity.

> >> diff --git a/fs/ioctl.c b/fs/ioctl.c
> >> index 2f5e4e5b97e1..f879aa94b118 100644
> >> --- a/fs/ioctl.c
> >> +++ b/fs/ioctl.c
> >> @@ -601,6 +601,11 @@ static int ioctl_fsfreeze(struct file *filp)
> >>  	if (sb->s_op->freeze_fs == NULL && sb->s_op->freeze_super == NULL)
> >>  		return -EOPNOTSUPP;
> >>  
> >> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> >> +	/* Freezing filesystems causes hung tasks. */
> >> +	return -EBUSY;
> >> +#endif
> > 
> > ick ick ick.
> 
> syzbot was failing to blacklist ioctl(FIFREEZE) due to a bug fixed at
> https://lore.kernel.org/linux-fsdevel/03633af8-fab8-a985-c215-f619ea4ded08@I-love.SAKURA.ne.jp/ .
> 
> Then, syzbot was not blacklisting ioctl(EXT4_IOC_SHUTDOWN) which was fixed at
> https://github.com/google/syzkaller/commit/61ed43a86a3721708aeeee72b23bfa1eacd921b2 .
> 
> These bugs could be fixed (in a whack-a-mole manner) but

No but.  Again, you are running a fuzzer as root.  You will end up doing
things that are bad.  Don't do those things.  Don't force the kernel to
keep track of the things you should not do, that's up to you and your
environment, not ours.

Maybe someone writes a fuzzer someday that does not need all of these
things, what then?  :)

> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 1ef6f75d92f1..9a2f95a78fef 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -1198,6 +1198,14 @@ MODULE_PARM_DESC(ignore_loglevel,
> >>  
> >>  static bool suppress_message_printing(int level)
> >>  {
> >> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> >> +	/*
> >> +	 * Changing console_loglevel causes "no output". But ignoring
> >> +	 * console_loglevel is easier than preventing change of
> >> +	 * console_loglevel.
> >> +	 */
> >> +	return (level >= CONSOLE_LOGLEVEL_DEFAULT && !ignore_loglevel);
> >> +#endif
> > 
> > I don't understand the need for this change at all.
> 
> this case was too hard to blacklist, as explained at
> https://lore.kernel.org/lkml/4d1a4b51-999b-63c6-5ce3-a704013cecb6@i-love.sakura.ne.jp/ .
> syz_execute_func() can find deeper bug by executing arbitrary binary code, but
> we cannot blacklist specific syscalls/arguments for syz_execute_func() testcases.
> Unless we guard on the kernel side, we won't be able to re-enable syz_execute_func()
> testcases.

Again, see above for what you shoudl have said.

And this still is really odd.

> >>  	return (level >= console_loglevel && !ignore_loglevel);
> >>  }
> >>  
> 
> Also, although ability to interpret kernel messages was improved by CONFIG_PRINTK_CALLER
> config option, we still cannot tell whether some string such as "BUG:" "WARNING:" came from
> BUG(), BUG_ON(), WARN(), WARN_ON() etc. Therefore, modules emitting such string causes
> syzbot to report as crash. For example, TOMOYO security module introduced
> CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING in order to allow syzbot to fuzz TOMOYO
> security module, and then extended CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING to
> prevent TOMOYO from emitting "WARNING:" string so that syzbot will not detect it as crash.

Removing WARNING strings is great, and is one thing and that kind of
thing has been merged as fixes for a while.  Messing with kernel log
levels is something totally different.

> As a result of these two patches, TOMOYO is now fuzzed by syzbot. But there are
> other modules emitting these strings. We need to somehow make it possible to
> distinguish whether these string came from BUG(), BUG_ON(), WARN(), WARN_ON() etc.
> in a generically applicable way.

Ok, but I don't see how the above log level change does that.

> To summarize, syzbot wants a kernel config option for two purposes:
> 
>   (1) Allow syzkaller run testcases as far/deep as possible.
> 
>   (2) Prevent syzkaller from generating false alarms.
> 
> This is beyond merely maintaining blacklist on the fuzzer side.
> 

As it is, this patch isn't acceptable at all for all of the reasons
above.

thanks,

greg k-h

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Andi Kleen]

Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes:

> While syzkaller is finding many bugs, sometimes syzkaller examines
> stupid operations. But disabling operations using kernel config option
> is problematic because "kernel config option excludes whole module when
> there is still room for examining all but specific operation" and
> "the list of kernel config options becomes too complicated to maintain
> because such list changes over time". Thus, 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.

It seems this should rather be a run time setting. Otherwise it's
impossible to fuzz existing kernel binaries without rebuilding them.

Yes syzkaller requires rebuilding anyways for the new compiler options,
but there are other options (e.g. PT guided fuzzing) that do not.

Maybe the run time setting can be the same as locked down kernels.
It seems to me locked down kernels should avoid all these operations too.

-Andi

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Greg Kroah-Hartman]

On Mon, Dec 16, 2019 at 10:34:55AM -0800, Andi Kleen wrote:
> Maybe the run time setting can be the same as locked down kernels.
> It seems to me locked down kernels should avoid all these operations too.

That's the same thing I said a ways back in the thread, but for some
unknown reason, no one seemed to like it :)

thanks,

greg k-h

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Theodore Y. Ts'o]

On Tue, Dec 17, 2019 at 12:35:00AM +0900, Tetsuo Handa wrote:
> >> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> >> index 1ef6f75d92f1..9a2f95a78fef 100644
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -1198,6 +1198,14 @@ MODULE_PARM_DESC(ignore_loglevel,
> >>  
> >>  static bool suppress_message_printing(int level)
> >>  {
> >> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> >> +	/*
> >> +	 * Changing console_loglevel causes "no output". But ignoring
> >> +	 * console_loglevel is easier than preventing change of
> >> +	 * console_loglevel.
> >> +	 */
> >> +	return (level >= CONSOLE_LOGLEVEL_DEFAULT && !ignore_loglevel);
> >> +#endif
> > 
> > I don't understand the need for this change at all.
> 
> this case was too hard to blacklist, as explained at
> https://lore.kernel.org/lkml/4d1a4b51-999b-63c6-5ce3-a704013cecb6@i-love.sakura.ne.jp/ .
> syz_execute_func() can find deeper bug by executing arbitrary binary code, but
> we cannot blacklist specific syscalls/arguments for syz_execute_func() testcases.
> Unless we guard on the kernel side, we won't be able to re-enable syz_execute_func()
> testcases.

I looked at the reference, but I didn't see the explanation in the
above link about why it was "too hard to blacklist".  In fact, it
looks like a bit earlier in the thread, Dmitry stated that adding this
find of blacklist "is not hard"?

https://lore.kernel.org/lkml/CACT4Y+Z_+H09iOPzSzJfs=_D=dczk22gL02FjuZ6HXO+p0kRyA@mail.gmail.com/

I suspect that adding whack-a-mole in the kernel is going to be just
as hard/annoying as adding it in Syzkaller....  The question is under
which rug are we proposing to hide the dirt?   :-)

      	      	 	      	       - Ted

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Tetsuo Handa]

On 2019/12/17 5:18, Theodore Y. Ts'o wrote:
>> this case was too hard to blacklist, as explained at
>> https://lore.kernel.org/lkml/4d1a4b51-999b-63c6-5ce3-a704013cecb6@i-love.sakura.ne.jp/ .
>> syz_execute_func() can find deeper bug by executing arbitrary binary code, but
>> we cannot blacklist specific syscalls/arguments for syz_execute_func() testcases.
>> Unless we guard on the kernel side, we won't be able to re-enable syz_execute_func()
>> testcases.
> 
> I looked at the reference, but I didn't see the explanation in the
> above link about why it was "too hard to blacklist".  In fact, it
> looks like a bit earlier in the thread, Dmitry stated that adding this
> find of blacklist "is not hard"?
> 
> https://lore.kernel.org/lkml/CACT4Y+Z_+H09iOPzSzJfs=_D=dczk22gL02FjuZ6HXO+p0kRyA@mail.gmail.com/
> 

That thread handled two bugs which disabled console output.

The former bug (March 2019) was that fuzzer was calling syslog(SYSLOG_ACTION_CONSOLE_LEVEL) and
was fixed by blacklisting syslog(SYSLOG_ACTION_CONSOLE_LEVEL). This case was easy to blacklist.

The latter bug (May 2019) was that fuzzer was calling binary code (via syz_execute_func()) which
emdebbed a system call that changes console loglevel. Since it was too difficult to blacklist,
syzkaller gave up use of syz_execute_func().

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Sergey Senozhatsky]

On (19/12/16 18:59), Tetsuo Handa wrote:
[..]
> +++ b/kernel/printk/printk.c
> @@ -1198,6 +1198,14 @@ MODULE_PARM_DESC(ignore_loglevel,
>  
>  static bool suppress_message_printing(int level)
>  {
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +	/*
> +	 * Changing console_loglevel causes "no output". But ignoring
> +	 * console_loglevel is easier than preventing change of
> +	 * console_loglevel.
> +	 */
> +	return (level >= CONSOLE_LOGLEVEL_DEFAULT && !ignore_loglevel);
> +#endif
>  	return (level >= console_loglevel && !ignore_loglevel);
>  }

Can you fuzz test with `ignore_loglevel'?

	-ss

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Masahiro Yamada]

On Mon, Dec 16, 2019 at 7:01 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>

Please do not use the subject prefix "kconfig:"
unless you are touching files in scripts/kconfig/.

Thanks.


> While syzkaller is finding many bugs, sometimes syzkaller examines
> stupid operations. But disabling operations using kernel config option
> is problematic because "kernel config option excludes whole module when
> there is still room for examining all but specific operation" and
> "the list of kernel config options becomes too complicated to maintain
> because such list changes over time". Thus, 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.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> ---
-- 
Best Regards
Masahiro Yamada

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Dmitry Vyukov]

On Tue, Dec 17, 2019 at 6:12 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (19/12/16 18:59), Tetsuo Handa wrote:
> [..]
> > +++ b/kernel/printk/printk.c
> > @@ -1198,6 +1198,14 @@ MODULE_PARM_DESC(ignore_loglevel,
> >
> >  static bool suppress_message_printing(int level)
> >  {
> > +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> > +     /*
> > +      * Changing console_loglevel causes "no output". But ignoring
> > +      * console_loglevel is easier than preventing change of
> > +      * console_loglevel.
> > +      */
> > +     return (level >= CONSOLE_LOGLEVEL_DEFAULT && !ignore_loglevel);
> > +#endif
> >       return (level >= console_loglevel && !ignore_loglevel);
> >  }
>
> Can you fuzz test with `ignore_loglevel'?

We can set ignore_loglevel in syzbot configs, but won't it then print
everything including verbose debug output?

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Sergey Senozhatsky]

On (19/12/17 08:54), Dmitry Vyukov wrote:
> On Tue, Dec 17, 2019 at 6:12 AM Sergey Senozhatsky
> <sergey.senozhatsky.work@gmail.com> wrote:
> >
> > On (19/12/16 18:59), Tetsuo Handa wrote:
> >
> > Can you fuzz test with `ignore_loglevel'?
> 
> We can set ignore_loglevel in syzbot configs, but won't it then print
> everything including verbose debug output?

What would be the most active source of debug output?

dev_dbg(), which is dev_printk(KERN_DEBUG), can be compiled out, if I'm
not mistaken. It probably depends on CONFIG_DEBUG or something similar,
unlike dev_printk(), which depends on CONFIG_PRINTK. It seems that we have
significantly more dev_dbg() users, than direct dev_printk(KERN_DEBUG).

Does fuzz tester hit pr_debug() often? File systems? Some of them
have ways to compile out debugging output as well. E.g. jbd_debug,
_debug, ext4_debug, and so on.

	-ss

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Dmitry Vyukov]

On Mon, Dec 16, 2019 at 10:07 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/12/17 5:18, Theodore Y. Ts'o wrote:
> >> this case was too hard to blacklist, as explained at
> >> https://lore.kernel.org/lkml/4d1a4b51-999b-63c6-5ce3-a704013cecb6@i-love.sakura.ne.jp/ .
> >> syz_execute_func() can find deeper bug by executing arbitrary binary code, but
> >> we cannot blacklist specific syscalls/arguments for syz_execute_func() testcases.
> >> Unless we guard on the kernel side, we won't be able to re-enable syz_execute_func()
> >> testcases.
> >
> > I looked at the reference, but I didn't see the explanation in the
> > above link about why it was "too hard to blacklist".  In fact, it
> > looks like a bit earlier in the thread, Dmitry stated that adding this
> > find of blacklist "is not hard"?
> >
> > https://lore.kernel.org/lkml/CACT4Y+Z_+H09iOPzSzJfs=_D=dczk22gL02FjuZ6HXO+p0kRyA@mail.gmail.com/
> >
>
> That thread handled two bugs which disabled console output.
>
> The former bug (March 2019) was that fuzzer was calling syslog(SYSLOG_ACTION_CONSOLE_LEVEL) and
> was fixed by blacklisting syslog(SYSLOG_ACTION_CONSOLE_LEVEL). This case was easy to blacklist.
>
> The latter bug (May 2019) was that fuzzer was calling binary code (via syz_execute_func()) which
> emdebbed a system call that changes console loglevel. Since it was too difficult to blacklist,
> syzkaller gave up use of syz_execute_func().


Yes, what Tetsuo says. Only syscall numbers and top-level arguments to
syscalls are easy to filter out. When indirect memory is passed to
kernel or (fd,ioctl) pairs are involved it boils down to solving the
halting problem.

There are some nice benefits of doing this in some form in kernel:
1. It makes reported crashes more trustworthy for kernel developers.
Currently you receive a crash and you don't know if it's a bug, or
working as intended (as turned out with serial console). It also
happened with syzkaller learning interesting ways to reach /dev/mem,
which was directly prohibited, but it managed to reach it via some
mounts and corrupted file names. Disabling the DEVMEM config in the
end reliably solved it once and for all.

2. It avoids duplication across test systems. Doing this in each test
system is again makes kernel testing hard and imposes duplication.
Tomorrow somebody runs new version of trinity, triforce, afl, perf
fuzzer and they hit the same issues, you get the same reports, and
have the same discussions and they slowly build the same list.

I like the idea of runtime knob that Andi proposed, and in fact this
looks quite similar to lockdown needs. And in fact it already have
LOCKDOWN_TIOCSSERIAL (does it does exactly the same as what we need?
if not, it may be something to improve in lockdown). It seems that any
fuzzing needs at least LOCKDOWN_INTEGRITY_MAX.

Could we incorporate some of the checks in this patch into lockdown?
FWIW we've just disabled sysrq entirely:
https://github.com/google/syzkaller/blob/master/dashboard/config/bits-syzbot.config#L182
because random packets over usb can trigger a panic sysrq (again
almost impossible to reliably filter these out on fuzzer side).

Not sure why lockdown prohibits parts of TIOCSSERIAL. Does it want to
prevent memory corruption, or also prevent turning off console output
(hiding traces)? If the latter then it may be reasonable to lockdown
lowering of console_loglevel.

But looks at some of the chunks here, it seems that we want something
slightly orthogonal to lockdown integrity/confidentiality, which
liveness/dos-prevention (can't accidentally bring down the whole
machine). E.g. FIFREEZE or bad SYSRQs.

There was some reason I did not enable lockdown when it was merged.
Now looking at the list again: LOCKDOWN_DEBUGFS is a no-go for us...

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Dmitry Vyukov]

On Tue, Dec 17, 2019 at 9:24 AM Sergey Senozhatsky
<sergey.senozhatsky.work@gmail.com> wrote:
>
> On (19/12/17 08:54), Dmitry Vyukov wrote:
> > On Tue, Dec 17, 2019 at 6:12 AM Sergey Senozhatsky
> > <sergey.senozhatsky.work@gmail.com> wrote:
> > >
> > > On (19/12/16 18:59), Tetsuo Handa wrote:
> > >
> > > Can you fuzz test with `ignore_loglevel'?
> >
> > We can set ignore_loglevel in syzbot configs, but won't it then print
> > everything including verbose debug output?
>
> What would be the most active source of debug output?
>
> dev_dbg(), which is dev_printk(KERN_DEBUG), can be compiled out, if I'm
> not mistaken. It probably depends on CONFIG_DEBUG or something similar,
> unlike dev_printk(), which depends on CONFIG_PRINTK. It seems that we have
> significantly more dev_dbg() users, than direct dev_printk(KERN_DEBUG).
>
> Does fuzz tester hit pr_debug() often? File systems? Some of them
> have ways to compile out debugging output as well. E.g. jbd_debug,
> _debug, ext4_debug, and so on.

As far as I remember it produces an infinite amount of output on debug
level. Even for normal level it produces much more than one would
normally expect as it does random things all over the kernel with
maximum frequency.

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Dmitry Vyukov]

On Mon, Dec 16, 2019 at 11:01 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> While syzkaller is finding many bugs, sometimes syzkaller examines
> stupid operations. But disabling operations using kernel config option
> is problematic because "kernel config option excludes whole module when
> there is still room for examining all but specific operation" and
> "the list of kernel config options becomes too complicated to maintain
> because such list changes over time". Thus, 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.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> ---
>  drivers/char/mem.c                  |  6 +++---
>  drivers/tty/serial/8250/8250_port.c |  7 +++++--
>  drivers/tty/vt/keyboard.c           |  3 +++
>  fs/ioctl.c                          |  5 +++++
>  include/linux/uaccess.h             | 12 ++++++++++++
>  kernel/printk/printk.c              |  8 ++++++++
>  lib/Kconfig.debug                   | 10 ++++++++++
>  lib/usercopy.c                      | 27 +++++++++++++++++++++++++++
>  8 files changed, 73 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> index 43dd0891ca1e..63aff3ae7c2b 100644
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -246,7 +246,7 @@ static ssize_t write_mem(struct file *file, const char __user *buf,
>                                 return -EFAULT;
>                         }
>
> -                       copied = copy_from_user(ptr, buf, sz);
> +                       copied = copy_from_user_to_any(ptr, buf, sz);

FWIW we disable /dev/{mem,kmem} in the config now:
https://github.com/google/syzkaller/blob/master/dashboard/config/bits-syzbot.config#L169
And this looks like a reasonable thing to do for any fuzzing.


>                         unxlate_dev_mem_ptr(p, ptr);
>                         if (copied) {
>                                 written += sz - copied;
> @@ -550,7 +550,7 @@ static ssize_t do_write_kmem(unsigned long p, const char __user *buf,
>                 if (!virt_addr_valid(ptr))
>                         return -ENXIO;
>
> -               copied = copy_from_user(ptr, buf, sz);
> +               copied = copy_from_user_to_any(ptr, buf, sz);
>                 if (copied) {
>                         written += sz - copied;
>                         if (written)
> @@ -604,7 +604,7 @@ static ssize_t write_kmem(struct file *file, const char __user *buf,
>                                 err = -ENXIO;
>                                 break;
>                         }
> -                       n = copy_from_user(kbuf, buf, sz);
> +                       n = copy_from_user_to_any(kbuf, buf, sz);
>                         if (n) {
>                                 err = -EFAULT;
>                                 break;
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 90655910b0c7..367b92ad598b 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -519,11 +519,14 @@ serial_port_out_sync(struct uart_port *p, int offset, int value)
>         case UPIO_MEM32:
>         case UPIO_MEM32BE:
>         case UPIO_AU:
> -               p->serial_out(p, offset, value);
> +               /* Writing to random kernel address causes crash. */
> +               if (!IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
> +                       p->serial_out(p, offset, value);

Does this do the same as LOCKDOWN_TIOCSSERIAL? How is it different?

>                 p->serial_in(p, UART_LCR);      /* safe, no side-effects */
>                 break;
>         default:
> -               p->serial_out(p, offset, value);
> +               if (!IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
> +                       p->serial_out(p, offset, value);
>         }
>  }
>
> diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c
> index 15d33fa0c925..367820b8ff59 100644
> --- a/drivers/tty/vt/keyboard.c
> +++ b/drivers/tty/vt/keyboard.c
> @@ -633,6 +633,9 @@ static void k_spec(struct vc_data *vc, unsigned char value, char up_flag)
>              kbd->kbdmode == VC_OFF) &&
>              value != KVAL(K_SAK))
>                 return;         /* SAK is allowed even in raw mode */
> +       /* Repeating SysRq-t forever causes RCU stalls. */
> +       if (IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
> +               return;
>         fn_handler[value](vc);
>  }
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 2f5e4e5b97e1..f879aa94b118 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -601,6 +601,11 @@ static int ioctl_fsfreeze(struct file *filp)
>         if (sb->s_op->freeze_fs == NULL && sb->s_op->freeze_super == NULL)
>                 return -EOPNOTSUPP;
>
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +       /* Freezing filesystems causes hung tasks. */
> +       return -EBUSY;
> +#endif
> +
>         /* Freeze */
>         if (sb->s_op->freeze_super)
>                 return sb->s_op->freeze_super(sb);
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 67f016010aad..6e5ddd0fdcce 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -387,4 +387,16 @@ void __noreturn usercopy_abort(const char *name, const char *detail,
>                                unsigned long len);
>  #endif
>
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +unsigned long __must_check copy_from_user_to_any(void *to,
> +                                                const void __user *from,
> +                                                unsigned long n);
> +#else
> +static __always_inline unsigned long __must_check
> +copy_from_user_to_any(void *to, const void __user *from, unsigned long n)
> +{
> +       return copy_from_user(to, from, n);
> +}
> +#endif
> +
>  #endif         /* __LINUX_UACCESS_H__ */
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 1ef6f75d92f1..9a2f95a78fef 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1198,6 +1198,14 @@ MODULE_PARM_DESC(ignore_loglevel,
>
>  static bool suppress_message_printing(int level)
>  {
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +       /*
> +        * Changing console_loglevel causes "no output". But ignoring
> +        * console_loglevel is easier than preventing change of
> +        * console_loglevel.
> +        */
> +       return (level >= CONSOLE_LOGLEVEL_DEFAULT && !ignore_loglevel);
> +#endif
>         return (level >= console_loglevel && !ignore_loglevel);
>  }
>
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index d1842fe756d5..f9836cc23942 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -2184,4 +2184,14 @@ config HYPERV_TESTING
>         help
>           Select this option to enable Hyper-V vmbus testing.
>
> +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
> diff --git a/lib/usercopy.c b/lib/usercopy.c
> index cbb4d9ec00f2..f7d5d243a63d 100644
> --- a/lib/usercopy.c
> +++ b/lib/usercopy.c
> @@ -86,3 +86,30 @@ int check_zeroed_user(const void __user *from, size_t size)
>         return -EFAULT;
>  }
>  EXPORT_SYMBOL(check_zeroed_user);
> +
> +#ifdef CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING
> +/*
> + * Since copying to arbitrary user-specified kernel address will crash
> + * the kernel trivially, do not copy to user-specified kernel address
> + * if the kernel was build for fuzz testing.
> + */
> +unsigned long __must_check copy_from_user_to_any(void *to,
> +                                                const void __user *from,
> +                                                unsigned long n)
> +{
> +       static char dummybuf[PAGE_SIZE];
> +       unsigned long ret = 0;
> +
> +       while (n) {
> +               unsigned long size = min(n, sizeof(dummybuf));
> +
> +               ret = copy_from_user(dummybuf, from, size);
> +               if (ret)
> +                       break;
> +               from += size;
> +               n -= size;
> +       }
> +       return ret;
> +}
> +EXPORT_SYMBOL(copy_from_user_to_any);
> +#endif
> --
> 2.18.1
>

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Dmitry Vyukov]

On Tue, Dec 17, 2019 at 9:36 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Dec 16, 2019 at 10:07 PM Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> >
> > On 2019/12/17 5:18, Theodore Y. Ts'o wrote:
> > >> this case was too hard to blacklist, as explained at
> > >> https://lore.kernel.org/lkml/4d1a4b51-999b-63c6-5ce3-a704013cecb6@i-love.sakura.ne.jp/ .
> > >> syz_execute_func() can find deeper bug by executing arbitrary binary code, but
> > >> we cannot blacklist specific syscalls/arguments for syz_execute_func() testcases.
> > >> Unless we guard on the kernel side, we won't be able to re-enable syz_execute_func()
> > >> testcases.
> > >
> > > I looked at the reference, but I didn't see the explanation in the
> > > above link about why it was "too hard to blacklist".  In fact, it
> > > looks like a bit earlier in the thread, Dmitry stated that adding this
> > > find of blacklist "is not hard"?
> > >
> > > https://lore.kernel.org/lkml/CACT4Y+Z_+H09iOPzSzJfs=_D=dczk22gL02FjuZ6HXO+p0kRyA@mail.gmail.com/
> > >
> >
> > That thread handled two bugs which disabled console output.
> >
> > The former bug (March 2019) was that fuzzer was calling syslog(SYSLOG_ACTION_CONSOLE_LEVEL) and
> > was fixed by blacklisting syslog(SYSLOG_ACTION_CONSOLE_LEVEL). This case was easy to blacklist.
> >
> > The latter bug (May 2019) was that fuzzer was calling binary code (via syz_execute_func()) which
> > emdebbed a system call that changes console loglevel. Since it was too difficult to blacklist,
> > syzkaller gave up use of syz_execute_func().
>
>
> Yes, what Tetsuo says. Only syscall numbers and top-level arguments to
> syscalls are easy to filter out. When indirect memory is passed to
> kernel or (fd,ioctl) pairs are involved it boils down to solving the
> halting problem.
>
> There are some nice benefits of doing this in some form in kernel:
> 1. It makes reported crashes more trustworthy for kernel developers.
> Currently you receive a crash and you don't know if it's a bug, or
> working as intended (as turned out with serial console). It also
> happened with syzkaller learning interesting ways to reach /dev/mem,
> which was directly prohibited, but it managed to reach it via some
> mounts and corrupted file names. Disabling the DEVMEM config in the
> end reliably solved it once and for all.
>
> 2. It avoids duplication across test systems. Doing this in each test
> system is again makes kernel testing hard and imposes duplication.
> Tomorrow somebody runs new version of trinity, triforce, afl, perf
> fuzzer and they hit the same issues, you get the same reports, and
> have the same discussions and they slowly build the same list.
>
> I like the idea of runtime knob that Andi proposed, and in fact this
> looks quite similar to lockdown needs. And in fact it already have
> LOCKDOWN_TIOCSSERIAL (does it does exactly the same as what we need?
> if not, it may be something to improve in lockdown). It seems that any
> fuzzing needs at least LOCKDOWN_INTEGRITY_MAX.
>
> Could we incorporate some of the checks in this patch into lockdown?
> FWIW we've just disabled sysrq entirely:
> https://github.com/google/syzkaller/blob/master/dashboard/config/bits-syzbot.config#L182
> because random packets over usb can trigger a panic sysrq (again
> almost impossible to reliably filter these out on fuzzer side).
>
> Not sure why lockdown prohibits parts of TIOCSSERIAL. Does it want to
> prevent memory corruption, or also prevent turning off console output
> (hiding traces)? If the latter then it may be reasonable to lockdown
> lowering of console_loglevel.
>
> But looks at some of the chunks here, it seems that we want something
> slightly orthogonal to lockdown integrity/confidentiality, which
> liveness/dos-prevention (can't accidentally bring down the whole
> machine). E.g. FIFREEZE or bad SYSRQs.
>
> There was some reason I did not enable lockdown when it was merged.
> Now looking at the list again: LOCKDOWN_DEBUGFS is a no-go for us...

+Matthew for a lockdown question
We are considering [ab]using lockdown (you knew this will happen!) for
fuzzing kernel. LOCKDOWN_DEBUGFS is a no-go for us and we may want a
few other things that may be fuzzing-specific.
The current inflexibility comes from the global ordering of levels:

if (kernel_locked_down >= level)
if (kernel_locked_down >= what) {

Is it done for performance? Or for simplicity?
If it would be more like an arbitrary bitset, then we could pick and
choose what we need and add few options on the side (not part of
integrity/confidentiality).
I see several potential ways to do it:
1. Rework the current code to allow arbitrary sets of options enabled.
2. Leave the current global ordering and checks intact and add a
separate mode that will do bitmask-like checks (e.g. under
CONFIG_LOCK_DOWN_FUZZER).
3. Or hardcode the fuzzer list statically based on config (but then we
can leave the rest of the code intact), e.g.:

#if !CONFIG_LOCK_DOWN_FUZZER
    [LOCKDOWN_DEBUGFS] = "debugfs access",
#endif
#if CONFIG_LOCK_DOWN_FUZZER
    [LOCKDOWN_FIFREEZE] = "FIFREEZE ioctl",
#endif

Thoughts?

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Tetsuo Handa]

On 2019/12/17 17:41, Dmitry Vyukov wrote:
>> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
>> index 90655910b0c7..367b92ad598b 100644
>> --- a/drivers/tty/serial/8250/8250_port.c
>> +++ b/drivers/tty/serial/8250/8250_port.c
>> @@ -519,11 +519,14 @@ serial_port_out_sync(struct uart_port *p, int offset, int value)
>>         case UPIO_MEM32:
>>         case UPIO_MEM32BE:
>>         case UPIO_AU:
>> -               p->serial_out(p, offset, value);
>> +               /* Writing to random kernel address causes crash. */
>> +               if (!IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
>> +                       p->serial_out(p, offset, value);
> 
> Does this do the same as LOCKDOWN_TIOCSSERIAL? How is it different?

I don't know. If there were an oversight in condition of lines 852-858,
uart_startup() might be called due to "goto check_and_exit;" without
hitting security_locked_down(LOCKDOWN_TIOCSSERIAL) check.

846:    old_flags = uport->flags;
847:    new_flags = (__force upf_t)new_info->flags;
848:    old_custom_divisor = uport->custom_divisor;
849:
850:    if (!capable(CAP_SYS_ADMIN)) {
851:            retval = -EPERM;
852:            if (change_irq || change_port ||
853:                (new_info->baud_base != uport->uartclk / 16) ||
854:                (close_delay != port->close_delay) ||
855:                (closing_wait != port->closing_wait) ||
856:                (new_info->xmit_fifo_size &&
857:                 new_info->xmit_fifo_size != uport->fifosize) ||
858:                (((new_flags ^ old_flags) & ~UPF_USR_MASK) != 0))
859:                    goto exit;
860:            uport->flags = ((uport->flags & ~UPF_USR_MASK) |
861:                           (new_flags & UPF_USR_MASK));
862:            uport->custom_divisor = new_info->custom_divisor;
863:            goto check_and_exit;
864:    }
865:
866:    retval = security_locked_down(LOCKDOWN_TIOCSSERIAL);
867:    if (retval && (change_irq || change_port))
868:            goto exit;

> 
>>                 p->serial_in(p, UART_LCR);      /* safe, no side-effects */
>>                 break;
>>         default:
>> -               p->serial_out(p, offset, value);
>> +               if (!IS_ENABLED(CONFIG_KERNEL_BUILT_FOR_FUZZ_TESTING))
>> +                       p->serial_out(p, offset, value);
>>         }
>>  }

But I came think that "BUG: unable to handle kernel NULL pointer dereference in
mem_serial_out" is a real kernel bug which should be fixed. It seems that crash
occurs only when "struct serial_struct"->iomem_base == NULL, and EBUSY is
returned otherwise. That is, some sanity check is wrong.

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Theodore Y. Ts'o]

On Tue, Dec 17, 2019 at 09:36:43AM +0100, Dmitry Vyukov wrote:
> Yes, what Tetsuo says. Only syscall numbers and top-level arguments to
> syscalls are easy to filter out. When indirect memory is passed to
> kernel or (fd,ioctl) pairs are involved it boils down to solving the
> halting problem.

I disagree that it's equivalent to solving the halting problem.
Otherwise, we couldn't filter in the kernel.  Let's think about ways
we can solve this.  One is to simply do what valgrind does; this
handles even self-modifying code, since you're essentially running an
x86-to-x86 emulator, and then you find an attempted trap to the
kernel, you can transfer control to a program which vets the arguments
to the system call.

Another approach might be to do this filtering in an BPF hook
installed at syscall entry.  Technically this is being done in the
kernel, but the advantage of this approach is that the BPF program can
be distributed alongside Syzkaller, and it can be Syzkaller-specific.
That way when we need to add a new blacklist entry, it can be done
without needing to wait for a kernel patch.

And note that there may *always* be some ioctls which we will need to
suppress.  For example, an attempt to send a SANITIZE ERASE to a
storage device; or an attempt to freeze the root file system, etc.
And I'm not sure all of these are ones that we can prevent by using
the lockdown setting.  There may very well be some commands that a
legitamate system administrator might want to execute that will, when
executed in the wrong circumstances causes the system to crash.  But
so long as it doesn't violate the trusted boot semantics which are the
whole point of lockdown, we would need to allow them.

So I suspect that some kind of filtering which is Syzkaller specific
is going to be inevitably needed, if you want to throw random binary
code and see what causes problem, and you insist on allowing these
random binary bits to be run as root.  Trying to prevent root from
being able to kill or self-DOS a machine goes way beyond any of our
current security mechanisms, and is something which is only really
needed by Fuzzers.  Personally, I suspect some kind of BPF filtering
is probably your best bet, since it will a bit more architecturally
portable than using some kind of Valgrind-like approach.  (Although
Valgrind *does* most of the architectures that I suspect we're going
to care about.)

						- Ted

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Tetsuo Handa]

On 2019/12/17 17:36, Dmitry Vyukov wrote:
> FWIW we've just disabled sysrq entirely:
> https://github.com/google/syzkaller/blob/master/dashboard/config/bits-syzbot.config#L182
> because random packets over usb can trigger a panic sysrq (again
> almost impossible to reliably filter these out on fuzzer side).

Excuse me, but CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE=0x0 helps only if show_state() etc. are
called via the __handle_sysrq() handler in drivers/tty/sysrq.c .

  static void sysrq_handle_showstate(int key)
  {
  	show_state();
  	show_workqueue_state();
  }
  static struct sysrq_key_op sysrq_showstate_op = {
  	.handler	= sysrq_handle_showstate,
  	.help_msg	= "show-task-states(t)",
  	.action_msg	= "Show State",
  	.enable_mask	= SYSRQ_ENABLE_DUMP,
  };

The k_spec() handler in drivers/tty/vt/keyboard.c calls show_state() etc. without
evaluating sysrq_enabled value.

  #define FN_HANDLERS\
  	fn_null,	fn_enter,	fn_show_ptregs,	fn_show_mem,\
  	fn_show_state,	fn_send_intr,	fn_lastcons,	fn_caps_toggle,\
  	fn_num,		fn_hold,	fn_scroll_forw,	fn_scroll_back,\
  	fn_boot_it,	fn_caps_on,	fn_compose,	fn_SAK,\
  	fn_dec_console, fn_inc_console, fn_spawn_con,	fn_bare_num
  
  typedef void (fn_handler_fn)(struct vc_data *vc);
  static fn_handler_fn FN_HANDLERS;
  static fn_handler_fn *fn_handler[] = { FN_HANDLERS };

  static void fn_show_state(struct vc_data *vc)
  {
  	show_state();
  }

  static void k_spec(struct vc_data *vc, unsigned char value, char up_flag)
  {
  	if (up_flag)
  		return;
  	if (value >= ARRAY_SIZE(fn_handler))
  		return;
  	if ((kbd->kbdmode == VC_RAW ||
  	     kbd->kbdmode == VC_MEDIUMRAW ||
  	     kbd->kbdmode == VC_OFF) &&
  	     value != KVAL(K_SAK))
  		return;		/* SAK is allowed even in raw mode */
  	fn_handler[value](vc);
  }

Therefore, we need to guard at either callee side (e.g. show_state_filter())
or caller side (e.g. k_spec()) using kernel config (or something equivalent)
in order to avoid forever calling show_state() from timer function.

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Dmitry Vyukov]

On Wed, Dec 18, 2019 at 11:30 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> On 2019/12/17 17:36, Dmitry Vyukov wrote:
> > FWIW we've just disabled sysrq entirely:
> > https://github.com/google/syzkaller/blob/master/dashboard/config/bits-syzbot.config#L182
> > because random packets over usb can trigger a panic sysrq (again
> > almost impossible to reliably filter these out on fuzzer side).
>
> Excuse me, but CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE=0x0 helps only if show_state() etc. are
> called via the __handle_sysrq() handler in drivers/tty/sysrq.c .
>
>   static void sysrq_handle_showstate(int key)
>   {
>         show_state();
>         show_workqueue_state();
>   }
>   static struct sysrq_key_op sysrq_showstate_op = {
>         .handler        = sysrq_handle_showstate,
>         .help_msg       = "show-task-states(t)",
>         .action_msg     = "Show State",
>         .enable_mask    = SYSRQ_ENABLE_DUMP,
>   };
>
> The k_spec() handler in drivers/tty/vt/keyboard.c calls show_state() etc. without
> evaluating sysrq_enabled value.
>
>   #define FN_HANDLERS\
>         fn_null,        fn_enter,       fn_show_ptregs, fn_show_mem,\
>         fn_show_state,  fn_send_intr,   fn_lastcons,    fn_caps_toggle,\
>         fn_num,         fn_hold,        fn_scroll_forw, fn_scroll_back,\
>         fn_boot_it,     fn_caps_on,     fn_compose,     fn_SAK,\
>         fn_dec_console, fn_inc_console, fn_spawn_con,   fn_bare_num
>
>   typedef void (fn_handler_fn)(struct vc_data *vc);
>   static fn_handler_fn FN_HANDLERS;
>   static fn_handler_fn *fn_handler[] = { FN_HANDLERS };
>
>   static void fn_show_state(struct vc_data *vc)
>   {
>         show_state();
>   }
>
>   static void k_spec(struct vc_data *vc, unsigned char value, char up_flag)
>   {
>         if (up_flag)
>                 return;
>         if (value >= ARRAY_SIZE(fn_handler))
>                 return;
>         if ((kbd->kbdmode == VC_RAW ||
>              kbd->kbdmode == VC_MEDIUMRAW ||
>              kbd->kbdmode == VC_OFF) &&
>              value != KVAL(K_SAK))
>                 return;         /* SAK is allowed even in raw mode */
>         fn_handler[value](vc);
>   }
>
> Therefore, we need to guard at either callee side (e.g. show_state_filter())
> or caller side (e.g. k_spec()) using kernel config (or something equivalent)
> in order to avoid forever calling show_state() from timer function.

+Andrey, please take a look if CONFIG_MAGIC_SYSRQ_DEFAULT_ENABLE
covers everything we need, or we need to disable sysrq entirely.

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Dmitry Vyukov]

On Tue, Dec 17, 2019 at 4:52 PM Theodore Y. Ts'o <tytso@mit.edu> wrote:
>
> On Tue, Dec 17, 2019 at 09:36:43AM +0100, Dmitry Vyukov wrote:
> > Yes, what Tetsuo says. Only syscall numbers and top-level arguments to
> > syscalls are easy to filter out. When indirect memory is passed to
> > kernel or (fd,ioctl) pairs are involved it boils down to solving the
> > halting problem.
>
> I disagree that it's equivalent to solving the halting problem.
> Otherwise, we couldn't filter in the kernel.  Let's think about ways
> we can solve this.  One is to simply do what valgrind does; this
> handles even self-modifying code, since you're essentially running an
> x86-to-x86 emulator, and then you find an attempted trap to the
> kernel, you can transfer control to a program which vets the arguments
> to the system call.

I don't know where to start :)

1. We don't run/have x86-to-x86 emulator and syzkaller is currently
supported on 6 architectures.
2. Complexity of this is very high (as compared to an if in kernel).
3. Valgrind-like solutions are a source of constant maintenance work
(we maintained valgrind for year at google).
4. Adding new architectures will be much harder.
5. All of this will need to be part of all C reproducers as well
(thousands of lines of intricate code, I think it was you who
complained about the complexity of even current C reproducers).
6. To not make this part of all reproducers we would need to run the
checking ahead of time (but this requires building complete and
precise kernel model and won't handle non-determinism; this is why I
referred to the "halting problem" assuming you don't want this in
reproducers).
7. This won't handle raciness between our checks and what kernel
really observes (i.e. we infer different fd type for ioctl, or we read
different data from memory).
8. This won't solve the problem of trust. If you receive this very
complex piece of code, will you be 100% sure that it's not syzkaller's
fault but a kernel bug that worth your time looking at.

So as far as I see this is both very complex and won't really work.

> Another approach might be to do this filtering in an BPF hook
> installed at syscall entry.  Technically this is being done in the
> kernel, but the advantage of this approach is that the BPF program can
> be distributed alongside Syzkaller, and it can be Syzkaller-specific.
> That way when we need to add a new blacklist entry, it can be done
> without needing to wait for a kernel patch.

This is subject to most of the same problem.
E.g. these BPF programs will need to be part of all reproducers, so
you will need to compile them for your kernel and install before
running reproducers. Also racy wrt what we observe and what kernel
acts on. Again some trust problems (it is still complex). Building and
using syzkaller will be harder.

We need to keep in mind that we are comparing this with is a simple if
in kernel code.


> And note that there may *always* be some ioctls which we will need to
> suppress.  For example, an attempt to send a SANITIZE ERASE to a
> storage device; or an attempt to freeze the root file system, etc.
> And I'm not sure all of these are ones that we can prevent by using
> the lockdown setting.  There may very well be some commands that a
> legitamate system administrator might want to execute that will, when
> executed in the wrong circumstances causes the system to crash.  But
> so long as it doesn't violate the trusted boot semantics which are the
> whole point of lockdown, we would need to allow them.
>
> So I suspect that some kind of filtering which is Syzkaller specific
> is going to be inevitably needed, if you want to throw random binary
> code and see what causes problem, and you insist on allowing these
> random binary bits to be run as root.  Trying to prevent root from
> being able to kill or self-DOS a machine goes way beyond any of our
> current security mechanisms, and is something which is only really
> needed by Fuzzers.  Personally, I suspect some kind of BPF filtering
> is probably your best bet, since it will a bit more architecturally
> portable than using some kind of Valgrind-like approach.  (Although
> Valgrind *does* most of the architectures that I suspect we're going
> to care about.)

We can easily filter out syscall numbers and top level syscall
argument values (executing random binary code aside, as we gave up on
this for now). That's what we use to filter out reboot syscalls and
FIFREEZE ioctl (fortunately the value does not collide with any other
ioctl we have _for now_). This is done by scanning the test case and
fixing it if necessary (all the necessary data is already there).

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Theodore Y. Ts'o]

On Thu, Dec 19, 2019 at 06:43:37PM +0100, Dmitry Vyukov wrote:
> We can easily filter out syscall numbers and top level syscall
> argument values (executing random binary code aside, as we gave up on
> this for now). That's what we use to filter out reboot syscalls and
> FIFREEZE ioctl (fortunately the value does not collide with any other
> ioctl we have _for now_). This is done by scanning the test case and
> fixing it if necessary (all the necessary data is already there).

OK, but a number of the changs made in the patch in question (such as
filtering out FIFREEZE by adding a hacky check in the kernel) was done
because the claim was made that Syzkaller *wanted* to run random
binary code.  If you given up for now, then maybe much or all of this
patch isn't needed?

If we do want to run random byte strings as instructions just to see
what the kernel will do, then some ugliness is going to be required.
The question is really, "where do we stash the ugliness", and who pays
the "ugliness tax", and are the benefits worth the costs?

    	      	    	    		       - Ted

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Matthew Garrett]

On Tue, Dec 17, 2019 at 12:53 AM Dmitry Vyukov <dvyukov@google.com> wrote:
> +Matthew for a lockdown question
> We are considering [ab]using lockdown (you knew this will happen!) for
> fuzzing kernel. LOCKDOWN_DEBUGFS is a no-go for us and we may want a
> few other things that may be fuzzing-specific.
> The current inflexibility comes from the global ordering of levels:
>
> if (kernel_locked_down >= level)
> if (kernel_locked_down >= what) {
>
> Is it done for performance? Or for simplicity?

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.

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Tetsuo Handa]

On 2020/01/03 4:57, Matthew Garrett wrote:
> On Tue, Dec 17, 2019 at 12:53 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>> +Matthew for a lockdown question
>> We are considering [ab]using lockdown (you knew this will happen!) for
>> fuzzing kernel. LOCKDOWN_DEBUGFS is a no-go for us and we may want a
>> few other things that may be fuzzing-specific.
>> The current inflexibility comes from the global ordering of levels:
>>
>> if (kernel_locked_down >= level)
>> if (kernel_locked_down >= what) {
>>
>> Is it done for performance? Or for simplicity?
> 
> 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.
> 

Can we resume this topic?

I think build-time lockdown (i.e. kernel config option) is more reliable
and easier to use.

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Tetsuo Handa]

On 2020/02/18 19:54, Tetsuo Handa wrote:
> On 2020/01/03 4:57, Matthew Garrett wrote:
>> On Tue, Dec 17, 2019 at 12:53 AM Dmitry Vyukov <dvyukov@google.com> wrote:
>>> +Matthew for a lockdown question
>>> We are considering [ab]using lockdown (you knew this will happen!) for
>>> fuzzing kernel. LOCKDOWN_DEBUGFS is a no-go for us and we may want a
>>> few other things that may be fuzzing-specific.
>>> The current inflexibility comes from the global ordering of levels:
>>>
>>> if (kernel_locked_down >= level)
>>> if (kernel_locked_down >= what) {
>>>
>>> Is it done for performance? Or for simplicity?
>>
>> 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.
>>
> 
> Can we resume this topic?
> 
> I think build-time lockdown (i.e. kernel config option) is more reliable
> and easier to use.
> 

Here is an example of need to lockdown specific ations. Can we proceed?

https://lkml.kernel.org/r/CACT4Y+azQXLcPqtJG9zbj8hxqw4jE3dcwUj5T06bdL3uMaZk+Q@mail.gmail.com

Re: [PATCH] kconfig: Add kernel config option for fuzz testing.

[Matthew Garrett]

On Thu, Feb 27, 2020 at 2:11 PM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:

> Here is an example of need to lockdown specific ations. Can we proceed?

As I said before, unless the thing being blocked is a primitive that's
intended to allow modification or reading of kernel memory (directly
or indirectly), I don't think lockdown is the right place for it to
be.