On Wed, Sep 12, 2018 at 8:30 PM, Dmitry Vyukov [off-list ref] wrote:

On Wed, Aug 29, 2018 at 1:35 PM, Andrey Konovalov [off-list ref] wrote:

quoted

 void kasan_unpoison_shadow(const void *address, size_t size)
 {
-       kasan_poison_shadow(address, size, 0);
+       u8 tag = get_tag(address);
+
+       /* Perform shadow offset calculation based on untagged address */

The comment is not super-useful. It would be more useful to say why we
need to do this.
Most callers explicitly untag pointer passed to this function, for
some it's unclear if the pointer contains tag or not.
For example, __hwasan_tag_memory -- what does it accept? Tagged or untagged?

There are some callers that pass tagged pointers to this functions,
e.g. ksize or kasan_unpoison_object_data. I'll expand the comment.

quoted

+       address = reset_tag(address);
+
+       kasan_poison_shadow(address, size, tag);

        if (size & KASAN_SHADOW_MASK) {
                u8 *shadow = (u8 *)kasan_mem_to_shadow(address + size);
-               *shadow = size & KASAN_SHADOW_MASK;
+
+               if (IS_ENABLED(CONFIG_KASAN_HW))
+                       *shadow = tag;
+               else
+                       *shadow = size & KASAN_SHADOW_MASK;
        }
 }

It seems that this function is just different for kasan and khwasan.
Currently for kasan we have:

kasan_poison_shadow(address, size, tag);
if (size & KASAN_SHADOW_MASK) {
        u8 *shadow = (u8 *)kasan_mem_to_shadow(address + size);
        *shadow = size & KASAN_SHADOW_MASK;
}

But what we want to say for khwasan is:

kasan_poison_shadow(address, round_up(size, KASAN_SHADOW_SCALE_SIZE),
get_tag(address));

Not sure if we want to keep a common implementation or just have
separate implementations...

As per offline discussion leaving as is.

quoted

 void kasan_free_pages(struct page *page, unsigned int order)

@@ -235,6 +248,7 @@ void kasan_cache_create(struct kmem_cache *cache, unsigned int *size,
                        slab_flags_t *flags)
 {
        unsigned int orig_size = *size;
+       unsigned int redzone_size = 0;

This variable seems to be always initialized below. We don't general
initialize local variables in this case.

Will fix in v7.

quoted

 void check_memory_region(unsigned long addr, size_t size, bool write,
                                unsigned long ret_ip)
 {
+       u8 tag;
+       u8 *shadow_first, *shadow_last, *shadow;
+       void *untagged_addr;
+
+       tag = get_tag((const void *)addr);
+
+       /* Ignore accesses for pointers tagged with 0xff (native kernel

/* on a separate line

Will fix in v7.

quoted

+        * pointer tag) to suppress false positives caused by kmap.
+        *
+        * Some kernel code was written to account for archs that don't keep
+        * high memory mapped all the time, but rather map and unmap particular
+        * pages when needed. Instead of storing a pointer to the kernel memory,
+        * this code saves the address of the page structure and offset within
+        * that page for later use. Those pages are then mapped and unmapped
+        * with kmap/kunmap when necessary and virt_to_page is used to get the
+        * virtual address of the page. For arm64 (that keeps the high memory
+        * mapped all the time), kmap is turned into a page_address call.
+
+        * The issue is that with use of the page_address + virt_to_page
+        * sequence the top byte value of the original pointer gets lost (gets
+        * set to KHWASAN_TAG_KERNEL (0xFF).

Missed closing bracket.

Will fix in v7.