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

[bugzilla-daemon@kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=216073

--- Comment #12 from 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

-- 
You may reply to this email to add a comment.

You are receiving this mail because:
You are watching someone on the CC list of the bug.