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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> ---
>  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
>