kmemcheck: trap uses of uninitialized memory (v2)

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.

arch/x86/kernel/kmemcheck_32.c new file +0 -0

new file mode 100644
index 0000000..9d065b9

arch/x86/kernel/kmemcheck_32.c new file +6 -0

+page_is_tracked(struct page *page)
+{
+    struct page *head;
+
+    if (!page)
+        return 0;

arch/x86/kernel/kmemcheck_32.c new file +60 -0

+}
+
+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) {

arch/x86/kernel/kmemcheck_32.c new file +15 -0

+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. */

arch/x86/kernel/kmemcheck_32.c new file +5 -0

+
+    /* Default operand size */
+    int operand_size_override = 32;
+
+    /* prefixes */

+opcode_get_primary(const uint8_t *opcode)

arch/x86/kernel/kmemcheck_32.c new file +23 -0

+
+    /* 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);
+

Quoted 1 line · Vegard Nossum

+static int

Not 'static bool'?

Quoted 1 line

+page_is_tracked(struct page *page)

Why not returning 'false' and 'true'?

arch/x86/kernel/kmemcheck_32.c line 114

Quoted 2 lines

arch/x86/kernel/kmemcheck_32.c new file +2 -0

+static unsigned int
+opcode_get_size(const uint8_t *opcode)

Are we not using 'u8' in the kernel?

cu
Richard Knutsson

Hi,

Quoted 6 lines · Richard Knutsson

Quoted 2 lines · Vegard Nossum

+static int

Not 'static bool'?

Quoted 4 lines

Quoted 2 lines

+page_is_tracked(struct page *page)

Why not returning 'false' and 'true'?

Sorry, I am not used to using bool in C :-) I will change this if bool is preferred in kernel code.

Quoted 5 lines

arch/x86/kernel/kmemcheck_32.c line 114

Quoted 3 lines

arch/x86/kernel/kmemcheck_32.c new file +2 -0

+static unsigned int
+opcode_get_size(const uint8_t *opcode)

Are we not using 'u8' in the kernel?

Actually, I don't see any reason to use u8 when uint8_t is already standard and used in other places in the kernel.

Hi Vegard,

arch/x86/Kconfig.debug config IOMMU_LEAK · line 123

Quoted 9 lines · Vegard Nossum

arch/x86/Kconfig.debug +9 -0

+config KMEMCHECK
+       bool "Trap use of uninitialized memory"
+       depends on X86_32 && !CC_OPTIMIZE_FOR_SIZE
+       help
+         This option enables tracing of dynamically allocated kernel memory
+         to see if memory is used before it has been given an initial value.
+         Be aware that this requires half of your memory for bookkeeping and
+         will insert extra code at *every* read and write to tracked memory
+         thus slow down the kernel code (but user code is unaffected).

Is it really necessary to track every memory address? Tracking slab objects would require far less memory. You might also want to make kzalloc() and GFP_ZERO mark the memory area as initialized to avoid some page faults.

mm/slub.c static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node) · line 1060

Quoted 3 lines

mm/slub.c +3 -0

+       /* Actually allocate twice as much, since we need to track the
+        * status of each byte within the allocation. */
+       if (!(flags & __GFP_NOTRACK)) {

If you change __GFP_NOTRACK to __GFP_TRACK, you can avoid the double negation here.

Pekka

Quoted 4 lines

Is it really necessary to track every memory address? Tracking slab objects would require far less memory. You might also want to make kzalloc() and GFP_ZERO mark the memory area as initialized to avoid some page faults.

Yes, we are in fact only tracking the memory within SLUB allocations (minus what SLUB itself needs for bookkeeping -- like the caches).

As for the kzalloc() and GFP_ZERO, I believe these will write zeros to the data in question before the memory is returned to the caller. In that case, the area will be "automatically" set to initialized since these writes are also intercepted by kmemcheck.

Hi,

Quoted 4 lines · Pekka Enberg

Quoted 4 lines

Is it really necessary to track every memory address? Tracking slab objects would require far less memory. You might also want to make kzalloc() and GFP_ZERO mark the memory area as initialized to avoid some page faults.

Quoted 2 lines · Vegard Nossum

Yes, we are in fact only tracking the memory within SLUB allocations (minus what SLUB itself needs for bookkeeping -- like the caches).

Yeah but you didn't answer my question: why do we track every memory address instead of slab objects? What's the benefit? Like I already said, tracking slab objects would require much less memory which makes the thing more practical. It also reduces the number of false positives (the CONFIG_OPTIMIZE_FOR_SIZE problem). And we already have slab poisoning to cover the cases we would not catch with this scheme.

Quoted 4 lines

As for the kzalloc() and GFP_ZERO, I believe these will write zeros to the data in question before the memory is returned to the caller. In that case, the area will be "automatically" set to initialized since these writes are also intercepted by kmemcheck.

Yes, and what I proposed is as a potential optimization. Debugging aids need to be fast enough to be practical.

Pekka

- We properly flush TLB entries that change. This used to not be the case, and so we

If I understand you correctly, you only want to be notified if any memory within an allocation is used before any memory within the allocation has been initialized. I think that this would be quite useless compared to tracking all the bytes of an allocation.

I am not truly concerned about the memory usage; this kind of error detection is by definition slow and memory intense.

Quoted 2 lines · Vegard Nossum

- We properly flush TLB entries that change. This used to not be the case, and so we

For low values of "properly" @)

arch/x86/kernel/kmemcheck_32.c line 41

Quoted 3 lines

arch/x86/kernel/kmemcheck_32.c new file +3 -0

+     pte = lookup_address(addr);
+     change_page_attr(page, 1, __pgprot(pte->pte_low | _PAGE_VISIBLE));
+     __flush_tlb_one(addr);

That's not enough, you need to flush all CPUs.

Also when you don't call global_flush_tlb() eventually because c_p_a() will leak flush objects over time.

-Andi

We don't need to flush all CPUs. This is my rationale: The debug exception (single-step trap) will always happen on the same CPU that the page fault occurred on. Page fault shows the page, debug exception hides the page again. Between those two operations, nothing else can

happen that will use the TLB entry in question (unless you have some weird race condition, but then the code is in error anyway).

What is c_p_a() and what is a flush object?