| 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
| ||
|
Message-ID: <CACT4Y+bDbHJ_NWcwoA6=MvpO9ptjU0yi9djoAWfS3Tx2Ltu4Ug@mail.gmail.com>
Date: Mon, 18 Jan 2016 12:21:44 +0100
From: Dmitry Vyukov <dvyukov@...gle.com>
To: "Kirill A. Shutemov" <kirill@...temov.name>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
David Drysdale <drysdale@...gle.com>,
Kees Cook <keescook@...gle.com>,
Quentin Casasnovas <quentin.casasnovas@...cle.com>,
Sasha Levin <sasha.levin@...cle.com>,
Vegard Nossum <vegard.nossum@...cle.com>,
LKML <linux-kernel@...r.kernel.org>,
Eric Dumazet <edumazet@...gle.com>,
Tavis Ormandy <taviso@...gle.com>,
Bjorn Helgaas <bhelgaas@...gle.com>,
syzkaller <syzkaller@...glegroups.com>,
Kostya Serebryany <kcc@...gle.com>,
Alexander Potapenko <glider@...gle.com>,
Andrey Ryabinin <ryabinin.a.a@...il.com>
Subject: Re: [PATCH v3] kernel: add kcov code coverage
On Fri, Jan 15, 2016 at 3:21 PM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
+simon.kagstrom
> On Fri, Jan 15, 2016 at 1:42 PM, Kirill A. Shutemov
> <kirill@...temov.name> wrote:
>> On Thu, Jan 14, 2016 at 03:22:21PM +0100, Dmitry Vyukov wrote:
>>> +#include <stdio.h>
>>> +#include <stddef.h>
>>> +#include <stdint.h>
>>> +#include <sys/types.h>
>>> +#include <sys/stat.h>
>>> +#include <sys/ioctl.h>
>>> +#include <sys/mman.h>
>>> +#include <fcntl.h>
>>> +
>>> +#define KCOV_INIT_TRACE _IOR('c', 1, unsigned long)
>>> +#define KCOV_ENABLE _IO('c', 100)
>>> +#define KCOV_DISABLE _IO('c', 101)
>>> +#define COVER_SIZE (64<<10)
>>> +
>>> +int main(int argc, char **argv)
>>> +{
>>> + int fd;
>>> + uint32_t *cover, n, i;
>>> +
>>> + /* A single fd descriptor allows coverage collection on a single
>>> + * thread.
>>> + */
>>> + fd = open("/sys/kernel/debug/kcov", O_RDWR);
>>> + if (fd == -1)
>>> + perror("open");
>>> + /* Setup trace mode and trace size. */
>>> + if (ioctl(fd, KCOV_INIT_TRACE, COVER_SIZE))
>>> + perror("ioctl");
>>> + /* Mmap buffer shared between kernel- and user-space. */
>>> + cover = (uint32_t*)mmap(NULL, COVER_SIZE * sizeof(uint32_t),
>>> + PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>>> + if ((void*)cover == MAP_FAILED)
>>> + perror("mmap");
>>> + /* Enable coverage collection on the current thread. */
>>> + if (ioctl(fd, KCOV_ENABLE, 0))
>>> + perror("ioctl");
>>> + /* Reset coverage from the tail of the ioctl() call. */
>>> + __atomic_store_n(&cover[0], 0, __ATOMIC_RELAXED);
>>> + /* That's the target syscal call. */
>>> + read(-1, NULL, 0);
>>> + /* Read number of PCs collected. */
>>> + n = __atomic_load_n(&cover[0], __ATOMIC_RELAXED);
>>> + /* PCs are shorten to uint32_t, so we need to restore the upper part. */
>>
>> Why do we do this in the first place? I don't think it's very portable.
>>
>> I would rather trancate upper bits on 32-bit system.
>
>
> Well, it is truncated to save time and space.
> On hot path we fill the buffer, sort, deduplicate, compare with
> another one. Plus I need to remember tens of thousands of such traces
> in memory.
>
> Any suggestions on how to keep compressed PCs for x86_64 and arm64
> while supporting archs where PCs don't fir into 32 bits?
>
>
>>> + for (i = 0; i < n; i++)
>>> + printf("0xffffffff%0lx\n", (unsigned long)cover[i + 1]);
>>> + /* Disable coverage collection for the current thread. After this call
>>> + * coverage can be enabled for a different thread.
>>> + */
>>> + if (ioctl(fd, KCOV_DISABLE, 0))
>>> + perror("ioctl");
>>> + /* Free resources. */
>>> + if (munmap(cover, COVER_SIZE * sizeof(uint32_t)))
>>> + perror("munmap");
>>> + if (close(fd))
>>> + perror("close");
>>> + return 0;
>>> +}
>>
>> ....
>>
>>> +static void kcov_unmap(struct vm_area_struct *vma)
>>> +{
>>> + kcov_put(vma->vm_file->private_data);
>>
>> Can be dropped, if I'm right about mmap counterpart.
>
> Great!
> Will drop all this in a next version.
>
>>> +}
>>> +
>>> +static void kcov_map_copied(struct vm_area_struct *vma)
>>> +{
>>> + struct kcov *kcov;
>>> +
>>> + kcov = vma->vm_file->private_data;
>>> + kcov_get(kcov);
>>
>> As with kcov_mmap(), I think this is redundant.
>>
>>> +}
>>> +
>>> +static const struct vm_operations_struct kcov_vm_ops = {
>>> + .fault = kcov_vm_fault,
>>> + .close = kcov_unmap,
>>> + /* Called on fork()/clone() when the mapping is copied. */
>>> + .open = kcov_map_copied,
>>> +};
>>> +
>>> +static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
>>> +{
>>> + int res = 0;
>>> + void *area;
>>> + struct kcov *kcov = vma->vm_file->private_data;
>>> +
>>> + area = vmalloc_user(vma->vm_end - vma->vm_start);
>>> + if (!area)
>>> + return -ENOMEM;
>>> +
>>> + spin_lock(&kcov->lock);
>>> + if (kcov->mode == 0 || vma->vm_pgoff != 0 ||
>>> + vma->vm_end - vma->vm_start != kcov->size * sizeof(u32)) {
>>> + res = -EINVAL;
>>> + goto exit;
>>> + }
>>> + if (!kcov->area) {
>>> + kcov->area = area;
>>> + area = NULL;
>>> + }
>>> + /*
>>> + * The file drops a reference on close, but the file
>>> + * descriptor can be closed with the mmaping still alive so we keep
>>> + * a reference for those. This is put in kcov_unmap().
>>
>> No, this is other way around: mmap takes file reference (see get_file() in
>> mmap_region()). So kcov_close() cannot happen, until last mmap gone.
>> I think this kcov_get() is redundant.
>>
>>> + */
>>> + kcov_get(kcov);
>>> + vma->vm_ops = &kcov_vm_ops;
>>> +exit:
>>> + spin_unlock(&kcov->lock);
>>> + vfree(area);
>>> + return res;
>>> +}
>>> +
>>> +static int kcov_open(struct inode *inode, struct file *filep)
>>> +{
>>> + struct kcov *kcov;
>>> +
>>> + kcov = kzalloc(sizeof(*kcov), GFP_KERNEL);
>>> + if (!kcov)
>>> + return -ENOMEM;
>>> + atomic_set(&kcov->rc, 1);
>>> + spin_lock_init(&kcov->lock);
>>> + filep->private_data = kcov;
>>> + return nonseekable_open(inode, filep);
>>> +}
>>> +
>>> +static int kcov_close(struct inode *inode, struct file *filep)
>>> +{
>>> + kcov_put(filep->private_data);
>>> + return 0;
>>> +}
>>> +
>>> +static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>>> + unsigned long arg)
>>> +{
>>> + struct task_struct *t;
>>> +
>>> + switch (cmd) {
>>> + case KCOV_INIT_TRACE:
>>> + /*
>>> + * Enable kcov in trace mode and setup buffer size.
>>> + * Must happen before anything else.
>>> + * Size must be at least 2 to hold current position and one PC.
>>> + */
>>> + if (arg < 2 || arg > INT_MAX)
>>> + return -EINVAL;
>>> + if (kcov->mode != 0)
>>> + return -EBUSY;
>>> + kcov->mode = kcov_mode_trace;
>>> + kcov->size = arg;
>>> + return 0;
>>> + case KCOV_ENABLE:
>>> + /*
>>> + * Enable coverage for the current task.
>>> + * At this point user must have been enabled trace mode,
>>> + * and mmapped the file. Coverage collection is disabled only
>>> + * at task exit or voluntary by KCOV_DISABLE. After that it can
>>> + * be enabled for another task.
>>> + */
>>> + if (kcov->mode == 0 || kcov->area == NULL)
>>> + return -EINVAL;
>>> + if (kcov->t != NULL)
>>> + return -EBUSY;
>>> + t = current;
>>> + /* Cache in task struct for performance. */
>>> + t->kcov_size = kcov->size;
>>> + t->kcov_area = kcov->area;
>>> + /* See comment in __sanitizer_cov_trace_pc(). */
>>> + barrier();
>>> + WRITE_ONCE(t->kcov_mode, kcov->mode);
>>> + t->kcov = kcov;
>>> + kcov->t = t;
>>> + /* This is put either in kcov_task_exit() or in KCOV_DISABLE. */
>>> + kcov_get(kcov);
>>> + return 0;
>>> + case KCOV_DISABLE:
>>> + /* Disable coverage for the current task. */
>>> + if (current->kcov != kcov)
>>> + return -EINVAL;
>>> + t = current;
>>> + if (WARN_ON(kcov->t != t))
>>> + return -EINVAL;
>>> + kcov_task_init(t);
>>> + kcov->t = NULL;
>>> + kcov_put(kcov);
>>> + return 0;
>>
>> You probably want to verify that arg is 0 for enable/disable just in case
>> if you would want to pass addtional information in the future.
>>
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +static long kcov_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>>> +{
>>> + struct kcov *kcov;
>>> + int res;
>>> +
>>> + kcov = filep->private_data;
>>> + spin_lock(&kcov->lock);
>>> + res = kcov_ioctl_locked(kcov, cmd, arg);
>>> + spin_unlock(&kcov->lock);
>>> + return res;
>>> +}
>>> +
>>> +static const struct file_operations kcov_fops = {
>>> + .open = kcov_open,
>>> + .unlocked_ioctl = kcov_ioctl,
>>> + .mmap = kcov_mmap,
>>> + .release = kcov_close,
>>> +};
>>> +
>>> +static int __init kcov_init(void)
>>> +{
>>> + if (!debugfs_create_file("kcov", 0666, NULL, NULL, &kcov_fops)) {
>>> + pr_err("init failed\n");
>>
>> I guess you should be more specific about which init failed.
>>
>>> + return 1;
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +device_initcall(kcov_init);
>> --
>> Kirill A. Shutemov
Powered by blists - more mailing lists