Re: [Bug 216073] New: [s390x] kernel BUG at mm/usercopy.c:101! usercopy: Kernel memory exposure attempt detected from vmalloc 'n o area' (offset 0, size 1)!

[Uladzislau Rezki <urezki@gmail.com>]

> On Sun, Jun 12, 2022 at 12:42:30PM +0800, Zorro Lang wrote:
> > Looks likt it's not a s390x specific bug, I just hit this issue once (not 100%
> > reproducible) on aarch64 with linux v5.19.0-rc1+ [1]. So back to cc linux-mm
> > to get more review.
> > 
> > [1]
> > [  980.200947] usercopy: Kernel memory exposure attempt detected from vmalloc 'no area' (offset 0, size 1)! 
> 
>        if (is_vmalloc_addr(ptr)) {
>                struct vm_struct *area = find_vm_area(ptr);
>                if (!area) {
>                        usercopy_abort("vmalloc", "no area", to_user, 0, n);
> 
> Oh.  Looks like XFS uses vm_map_ram() and vm_map_ram() doesn't allocate
> a vm_struct.
> 
> Ulad, how does this look to you?
>
It looks like a correct way to me :) XFS uses per-cpu-vm_map_ram()-vm_unmap_ram()
API which do not allocate "vm_struct" because it is not needed.

>
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index baeacc735b83..6bc2a1407c59 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -173,7 +173,7 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
>  	}
>  
>  	if (is_vmalloc_addr(ptr)) {
> -		struct vm_struct *area = find_vm_area(ptr);
> +		struct vmap_area *area = find_vmap_area((unsigned long)ptr);
>  		unsigned long offset;
>  
>  		if (!area) {
> @@ -181,8 +181,9 @@ static inline void check_heap_object(const void *ptr, unsigned long n,
>  			return;
>  		}
>  
> -		offset = ptr - area->addr;
> -		if (offset + n > get_vm_area_size(area))
> +		/* XXX: We should also abort for free vmap_areas */
> +		offset = (unsigned long)ptr - area->va_start;
>
I was a bit confused about "offset" and why it is needed here. It is always zero. 
So we can get rid of it to make it less confused. From the other hand a zero offset
contributes to nothing.

>
> +		if (offset + n >= area->va_end)
>
I think it is a bit wrong. As i see it, "n" is a size and what we would like to do
here is boundary check:

<snip>
if (n > va_size(area))
    usercopy_abort("vmalloc", NULL, to_user, 0, n);
<snip>

--
Uladzislau Rezki