linux-kernel - Re: [PATCH 1/2] kcov: size of arena is now given in bytes.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Nov 2016 08:54:36 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Quentin Casasnovas <quentin.casasnovas@...cle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Vegard Nossum <vegard.nossum@...cle.com>,
        Michal Zalewski <lcamtuf@...il.com>,
        Kees Cook <keescook@...omium.org>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: [PATCH 1/2] kcov: size of arena is now given in bytes.

On Wed, Nov 16, 2016 at 10:27 PM, Quentin Casasnovas
<quentin.casasnovas@...cle.com> wrote:
> We'll introduce a different mode of tracing a-la AFL fixed table and Dmitry
> suggests that the code would be simpler with the size expressed in bytes as
> opposed unsigned longs.
>
> We only change the kcov::size field, which will be shared between different
> modes, but leave the task_struct::kcov_size field expressed in unsigned
> long in order to save an unecessary bitshift/division in the hot path when
> using KCOV_MODE_TRACE.


Thanks!

Reviewed-and-tested-by: Dmitry Vyukov <dvyukov@...gle.com>

I've tested that KCOV_MODE_TRACE works for me with these two patches applied.

> but leave the task_struct::kcov_size field expressed in unsigned long

The only purpose of the cache in task struct is to make coverage fast
path fast, so this looks good to me.



> Cc: Dmitry Vyukov <dvyukov@...gle.com>
> Cc: Michal Zalewski <lcamtuf@...il.com>
> Cc: Kees Cook <keescook@...omium.org>
> Signed-off-by: Quentin Casasnovas <quentin.casasnovas@...cle.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@...cle.com>
> ---
>  kernel/kcov.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 30e6d05aa5a9..c2aa93851f93 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -32,7 +32,7 @@ struct kcov {
>         /* The lock protects mode, size, area and t. */
>         spinlock_t              lock;
>         enum kcov_mode          mode;
> -       /* Size of arena (in long's for KCOV_MODE_TRACE). */
> +       /* Size of arena in bytes. */
>         unsigned                size;
>         /* Coverage buffer shared with user space. */
>         void                    *area;
> @@ -140,7 +140,7 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>                 return -ENOMEM;
>
>         spin_lock(&kcov->lock);
> -       size = kcov->size * sizeof(unsigned long);
> +       size = kcov->size;
>         if (kcov->mode == KCOV_MODE_DISABLED || vma->vm_pgoff != 0 ||
>             vma->vm_end - vma->vm_start != size) {
>                 res = -EINVAL;
> @@ -198,13 +198,11 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>                         return -EBUSY;
>                 /*
>                  * Size must be at least 2 to hold current position and one PC.
> -                * Later we allocate size * sizeof(unsigned long) memory,
> -                * that must not overflow.
>                  */
>                 size = arg;
>                 if (size < 2 || size > INT_MAX / sizeof(unsigned long))
>                         return -EINVAL;
> -               kcov->size = size;
> +               kcov->size = size * sizeof(unsigned long);
>                 kcov->mode = KCOV_MODE_TRACE;
>                 return 0;
>         case KCOV_ENABLE:
> @@ -223,7 +221,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>                         return -EBUSY;
>                 t = current;
>                 /* Cache in task struct for performance. */
> -               t->kcov_size = kcov->size;
> +               t->kcov_size = kcov->size / sizeof(unsigned long);
>                 t->kcov_area = kcov->area;
>                 /* See comment in __sanitizer_cov_trace_pc(). */
>                 barrier();
> --
> 2.10.2
>