Re: [RFC] kmemcheck: trap uses of uninitialized memory (v2)

[Richard Knutsson <ricknu-0@student.ltu.se>]

Vegard Nossum wrote:
> General description: kmemcheck will trap every read and write to 
> memory that was
> allocated dynamically (ie. with kmalloc()). If a memory address is 
> read that has
> not previously been written to, a message is printed to the kernel log.
>
<snip>
> diff --git a/arch/x86/kernel/kmemcheck_32.c 
> b/arch/x86/kernel/kmemcheck_32.c
> new file mode 100644
> index 0000000..9d065b9
> --- /dev/null
> +++ b/arch/x86/kernel/kmemcheck_32.c
> @@ -0,0 +1,290 @@
> +#include <linux/kernel.h>
> +#include <linux/kallsyms.h>
> +#include <linux/mm.h>
> +#include <asm/cacheflush.h>
> +#include <asm/kmemcheck.h>
> +#include <asm/tlbflush.h>
> +
> +static int
Not 'static bool'?
> +page_is_tracked(struct page *page)
> +{
> +    struct page *head;
> +
> +    if (!page)
> +        return 0;
> +    head = compound_head(page);
> +    if (!head)
> +        return 0;
> +    if (!(head->flags & (1 << PG_slab)))
> +        return 0;
> +    if (!head->slab)
> +        return 0;
> +    if (head->slab->flags & __GFP_NOTRACK)
> +        return 0;
> +    return 1;
Why not returning 'false' and 'true'?
> +}
> +
> +static void
> +show_addr(uint32_t addr)
> +{
> +    struct page *page;
> +    pte_t *pte;
> +
> +    if (!addr)
> +        return;
> +    page = virt_to_page(addr);
> +    if (!page_is_tracked(page))
> +        return;
> +
> +    pte = lookup_address(addr);
> +    change_page_attr(page, 1, __pgprot(pte->pte_low | _PAGE_VISIBLE));
> +    __flush_tlb_one(addr);
> +}
> +
> +static void
> +hide_addr(uint32_t addr)
> +{
> +    struct page *page;
> +    pte_t *pte;
> +
> +    if (!addr)
> +        return;
> +    page = virt_to_page(addr);
> +    if (!page_is_tracked(page))
> +        return;
> +    pte = lookup_address(addr);
> +    change_page_attr(page, 1, __pgprot(pte->pte_low & ~_PAGE_VISIBLE));
> +    __flush_tlb_one(addr);
> +}
> +
> +DEFINE_PER_CPU(uint32_t, kmemcheck_read_addr);
> +DEFINE_PER_CPU(uint32_t, kmemcheck_write_addr);
> +
> +void
> +kmemcheck_show(struct pt_regs *regs)
> +{
> +    show_addr(__get_cpu_var(kmemcheck_read_addr));
> +    show_addr(__get_cpu_var(kmemcheck_write_addr));
> +    regs->eflags |= TF_MASK;
> +}
> +
> +void
> +kmemcheck_hide(struct pt_regs *regs)
> +{
> +    hide_addr(__get_cpu_var(kmemcheck_read_addr));
> +    hide_addr(__get_cpu_var(kmemcheck_write_addr));
> +    regs->eflags &= ~TF_MASK;
> +}
> +
> +void
> +kmemcheck_hide_pages(struct page *p, unsigned int n)
> +{
> +    unsigned int i;
> +
> +    for(i = 0; i < n; ++i) {
> +        unsigned long address = (unsigned long) page_address(&p[i]);
> +        pte_t *pte = lookup_address(address);
> +
> +        change_page_attr(&p[i], 1,
> +            __pgprot(pte->pte_low & ~_PAGE_VISIBLE));
> +        __flush_tlb_one(address);
> +    }
> +}
> +
> +static int
'static bool'?
> +opcode_is_prefix(uint8_t b)
> +{
> +    return
> +        /* Group 1 */
> +        b == 0xf0 || b == 0xf2 || b == 0xf3
> +        /* Group 2 */
> +        || b == 0x2e || b == 0x36 || b == 0x3e || b == 0x26
> +        || b == 0x64 || b == 0x65 || b == 0x2e || b == 0x3e
> +        /* Group 3 */
> +        || b == 0x66
> +        /* Group 4 */
> +        || b == 0x67;
> +}
> +
> +/* This is a VERY crude opcode decoder. We only need to find the size 
> of the
> + * load/store that caused our #PF and this should work for all the 
> opcodes
> + * that we care about. Moreover, the ones who invented this 
> instruction set
> + * should be shot. */
> +static unsigned int
> +opcode_get_size(const uint8_t *opcode)
Are we not using 'u8' in the kernel?
> +{
> +    const uint8_t *i;
and here. Also, I find the name 'i' a bit confusing in this context, 
since it is not really a counter. What about 'cur', 'prefix_op' or 
something of the sort?
> +
> +    /* Default operand size */
> +    int operand_size_override = 32;
> +
> +    /* prefixes */
> +    for (i = opcode; opcode_is_prefix(*i); ++i) {
> +        if (*i == 0x66)
> +            operand_size_override = 16;
> +    }
> +
> +    /* escape opcode */
> +    if (*i == 0x0f) {
> +        ++i;
> +
> +        if(*i == 0xb6)
Please do, as the previous, 'if ()'. A good checker for these kind of 
things is the 'scripts/checkpatch.pl'...
> +            return operand_size_override >> 1;
> +        if(*i == 0xb7)
> +            return 16;
> +    }
> +
> +    return (*i & 1) ? operand_size_override : 8;
> +}
> +
> +static uint8_t
and here...
> +opcode_get_primary(const uint8_t *opcode)
and here..
> +{
> +    const uint8_t *i;
and here. Also, I find the name 'i' a bit confusing in this context, 
since it is not really a counter. What about 'cur', 'prim_op', 
'prefix_op' or something of the sort?
> +
> +    /* skip prefixes */
> +    for (i = opcode; opcode_is_prefix(*i); ++i);
> +    return *i;
> +}
> +
> +static void *address_get_shadow_slab(unsigned long address,
> +    struct page *head)
> +{
> +    if (!head->slab)
> +        return NULL;
> +
> +    if (head->slab->flags & __GFP_NOTRACK)
> +        return NULL;
> +
> +    return (void *) address + (PAGE_SIZE << head->slab->order);
> +}
> +
> +static void *address_get_shadow(unsigned long address)
> +{
> +    struct page *page = virt_to_page(address);
> +    struct page *head = compound_head(page);
> +
> +    if (!head)
> +        return NULL;
> +
> +    if (head->flags & (1 << PG_slab))
> +        return address_get_shadow_slab(address, head);
> +
> +    return NULL;
> +}
> +
> +static int
'static bool'?
> +test(void *shadow, unsigned int size)
> +{
> +    switch (size) {
> +    case 8:
> +        return *(uint8_t *) shadow == 0xff;
> +    case 16:
> +        return *(uint16_t *) shadow == 0xffff;
> +    case 32:
> +        return *(uint32_t *) shadow == 0xffffffff;
> +    }
> +
> +    BUG();
> +    return 0;
'return false;'?
> +}
> +
<snip>

cu
Richard Knutsson