Re: [PATCH v3 1/2] MIPS: Don't crash in stack_top() for tasks without ABI or vDSO

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

On Tue, 15 Apr 2025 at 15:10, Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> Not all tasks have an ABI associated or vDSO mapped,
> for example kthreads never do.
> If such a task ever ends up calling stack_top(), it will derefence the
> NULL ABI pointer and crash.
>
> This can for example happen when using kunit:
>
>     mips_stack_top+0x28/0xc0
>     arch_pick_mmap_layout+0x190/0x220
>     kunit_vm_mmap_init+0xf8/0x138
>     __kunit_add_resource+0x40/0xa8
>     kunit_vm_mmap+0x88/0xd8
>     usercopy_test_init+0xb8/0x240
>     kunit_try_run_case+0x5c/0x1a8
>     kunit_generic_run_threadfn_adapter+0x28/0x50
>     kthread+0x118/0x240
>     ret_from_kernel_thread+0x14/0x1c
>
> Only dereference the ABI point if it is set.
>
> Also move the randomization adjustment into the same conditional.
>
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> ---

This looks good to me -- we've hit what's effectively the same issue
on other architectures with this test, with similar resolutions.

Reviewed-by: David Gow <davidgow@google.com>

I'm happy to have this (and the following path) go in via the KUnit
tree, but if you'd rather have it go via MIPS, that's fine, too. (The
qemu_configs won't generate any merge conflicts.)

-- David

>  arch/mips/kernel/process.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
> index b630604c577f9ff3f2493b0f254363e499c8318c..02aa6a04a21da437909eeac4f149155cc298f5b5 100644
> --- a/arch/mips/kernel/process.c
> +++ b/arch/mips/kernel/process.c
> @@ -690,18 +690,20 @@ unsigned long mips_stack_top(void)
>         }
>
>         /* Space for the VDSO, data page & GIC user page */
> -       top -= PAGE_ALIGN(current->thread.abi->vdso->size);
> -       top -= PAGE_SIZE;
> -       top -= mips_gic_present() ? PAGE_SIZE : 0;
> +       if (current->thread.abi) {
> +               top -= PAGE_ALIGN(current->thread.abi->vdso->size);
> +               top -= PAGE_SIZE;
> +               top -= mips_gic_present() ? PAGE_SIZE : 0;
> +
> +               /* Space to randomize the VDSO base */
> +               if (current->flags & PF_RANDOMIZE)
> +                       top -= VDSO_RANDOMIZE_SIZE;
> +       }
>
>         /* Space for cache colour alignment */
>         if (cpu_has_dc_aliases)
>                 top -= shm_align_mask + 1;
>
> -       /* Space to randomize the VDSO base */
> -       if (current->flags & PF_RANDOMIZE)
> -               top -= VDSO_RANDOMIZE_SIZE;
> -
>         return top;
>  }
>
>
> --
> 2.49.0
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5281 bytes --]