On 22/07/26 7:59 pm, Dev Jain wrote:
> The vmalloc allocator stores the actual allocation size inside the
> vm_struct structure. We can use this bound in usercopy instead of the
> page-aligned va_end to catch usercopy beyond the actual allocation size.
>
> For vmap, the requested_size field is always page-aligned since it maps a
> certain number of pages. Same for vm_map_ram (alongwith, not even having
> a vm_struct). So the check is only relevant for vmalloc mappings.
>
> Because there are early vm areas registered even before vmalloc_init,
> requested_size may be zero. So also check whether the requested_size
> is set.
>
> Signed-off-by: Dev Jain <[email protected]>
> ---
Sashiko:
1. "Does this locklessly access area->vm after find_vmap_area() has dropped
the busy tree lock?
If an out-of-bounds pointer falls into an adjacent vmap_area, and that
adjacent area is concurrently freed by another thread, its vm_struct
is freed. Additionally, when the vmap_area is moved to the free tree,
area->vm (which shares a union with subtree_max_size) is overwritten
with an integer size.
Would dereferencing vm->flags later in this function cause a use-after-free
or a wild pointer dereference?"
I don't get it. So usercopy is checking OOB for an object but shouldn't
assume the existence of that object while using it?
It is a bug in the caller if someone does vfree() while usercopy is operating
on the vmalloc object. I don't think usercopy should handle it. For example
we don't handle it for slabs currently.
2. "Can addr still have hardware KASAN tags (like ARM64 MTE) attached in the
top byte here?
Because check_heap_object() casts ptr to addr without calling
kasan_reset_tag(), a tagged address will be numerically different from
untagged kernel addresses. When calculating offset against the untagged
area->va_start, this could underflow or produce a massive unsigned value.
Could this cause the offset > size check to evaluate to true, triggering a
spurious usercopy_abort() for valid usercopies on HW-tagged vmalloc memory?"
This looks genuine. check_heap_object() should be doing a kasan_reset_tag().
We probably didn't catch this because no one is running CONFIG_HARDENED_USERCOPY
with KASAN enabled, since the latter kind-of already stops the OOB bugs.
Even if they did, mostly we care about slab OOB's I think, and the
__check_heap_object
in mm/slub.c does a kasan_reset_tag() already.
> Applies on mm-new (3d18f3499c48).
>
> mm/usercopy.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 5de7a518b1b1c..772681aff8477 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -176,10 +176,28 @@ static inline void check_heap_object(const void *ptr,
> unsigned long n,
>
> if (is_vmalloc_addr(ptr) && !pagefault_disabled()) {
> struct vmap_area *area = find_vmap_area(addr);
> + struct vm_struct *vm;
>
> if (!area)
> usercopy_abort("vmalloc", "no area", to_user, 0, n);
>
> + vm = area->vm;
> + /*
> + * Mappings with a vm_struct track the originally requested
> + * size. Check against that rather than the page-rounded
> + * vmap_area->va_end so copies cannot reach vmalloc tail
> + * padding. vmap mappings are always page aligned.
> + */
> + if (vm && (vm->flags & VM_ALLOC) && vm->requested_size) {
> + unsigned long size = vm->requested_size;
> +
> + offset = addr - area->va_start;
> + if (offset > size || n > size - offset)
> + usercopy_abort("vmalloc", NULL, to_user,
> + offset, n);
> + return;
> + }
> +
> if (n > area->va_end - addr) {
> offset = addr - area->va_start;
> usercopy_abort("vmalloc", NULL, to_user, offset, n);
