On Fri, Oct 30, 2020 at 2:46 PM Marco Elver [off-list ref] wrote:

On Fri, 30 Oct 2020 at 03:50, Jann Horn [off-list ref] wrote:

quoted

On Thu, Oct 29, 2020 at 2:17 PM Marco Elver [off-list ref] wrote:

quoted

We make KFENCE compatible with KASAN for testing KFENCE itself. In
particular, KASAN helps to catch any potential corruptions to KFENCE
state, or other corruptions that may be a result of freepointer
corruptions in the main allocators.

To indicate that the combination of the two is generally discouraged,
CONFIG_EXPERT=y should be set. It also gives us the nice property that
KFENCE will be build-tested by allyesconfig builds.

Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Co-developed-by: Marco Elver <elver@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>

Reviewed-by: Jann Horn <jannh@google.com>

Thanks!

quoted

with one nit:

[...]

quoted

diff --git a/mm/kasan/common.c b/mm/kasan/common.c

[...]

quoted

@@ -141,6 +142,14 @@ void kasan_unpoison_shadow(const void *address, size_t size)
         */
        address = reset_tag(address);

+       /*
+        * We may be called from SL*B internals, such as ksize(): with a size
+        * not a multiple of machine-word size, avoid poisoning the invalid
+        * portion of the word for KFENCE memory.
+        */
+       if (is_kfence_address(address))
+               return;

It might be helpful if you could add a comment that explains that
kasan_poison_object_data() does not need a similar guard because
kasan_poison_object_data() is always paired with
kasan_unpoison_object_data() - that threw me off a bit at first.

Well, KFENCE objects should never be poisoned/unpoisoned because the
kasan_alloc and free hooks have a kfence guard, and none of the code
in sl*b.c that does kasan_{poison,unpoison}_object_data() should be
executed for KFENCE objects.

But I just noticed that kernel/scs.c seems to kasan_poison and
unpoison objects, and keeps them poisoned for most of the object
lifetime.

FWIW, I wouldn't be surprised if other parts of the kernel also ended
up wanting to have in-object redzones eventually - e.g. inside skb
buffers, which have a struct skb_shared_info at the end. AFAIU at the
moment, KASAN can't catch small OOB accesses from these buffers
because of the following structure.

I think we better add a kfence guard to
kasan_poison_shadow() as well.

Sounds good.