Re: [PATCH] mm/usercopy: harden bounds checking for vmalloc allocations

On 26/07/26 2:23 pm, Dev Jain wrote:
> 
> 
> 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.


To be clear, this is an existing bug, so shouldn't stop this patch from going in.