Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in the vmalloc area

[David Gow]

On Fri, 23 Feb 2024 at 22:07, Petr Tesarik <petrtesarik@xxxxxxxxxxxxxxx> wrote:
>
> From: Petr Tesarik <petr.tesarik1@xxxxxxxxxxxxxxxxxxx>
>
> If a segmentation fault is caused by accessing an address in the vmalloc
> area, check that the target page is present.
>
> Currently, if the kernel hits a guard page in the vmalloc area, UML blindly
> assumes that the fault is caused by a stale mapping and will be fixed by
> flush_tlb_kernel_vm(). Unsurprisingly, if the fault is caused by accessing
> a guard page, no mapping is created, and when the faulting instruction is
> restarted, it will cause exactly the same fault again, effectively creating
> an infinite loop.
>
> Signed-off-by: Petr Tesarik <petr.tesarik1@xxxxxxxxxxxxxxxxxxx>
> ---
>  arch/um/kernel/trap.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
> index 6d8ae86ae978..d5b85f1bfe33 100644
> --- a/arch/um/kernel/trap.c
> +++ b/arch/um/kernel/trap.c
> @@ -206,11 +206,15 @@ unsigned long segv(struct faultinfo fi, unsigned long ip, int is_user,
>         int err;
>         int is_write = FAULT_WRITE(fi);
>         unsigned long address = FAULT_ADDRESS(fi);
> +       pte_t *pte;
>
>         if (!is_user && regs)
>                 current->thread.segv_regs = container_of(regs, struct pt_regs, regs);
>
>         if (!is_user && (address >= start_vm) && (address < end_vm)) {
> +               pte = virt_to_pte(&init_mm, address);
> +               if (!pte_present(*pte))
> +                       page_fault_oops(regs, address, ip);

page_fault_oops() appears to be private to arch/x86/mm/fault.c, so
can't be used here?
Also, it accepts struct pt_regs*, not struct uml_pt_regs*, so would
need to at least handle the type difference here.

Could we equally avoid the infinite loop here by putting the
'flush_tlb_kernel_vm();goto out;' behind a if (pte_present(...))
check, and let the rest of the UML checks panic or oops if required.
(Actually OOPSing where we can under UML would be nice to do at some
point anyway, but is a bigger issue than just fixing a bug, IMO.)

Or am I lacking a prerequisite patch or applying this to the wrong
version (or otherwise missing something), as it definitely doesn't
build here.

Cheers,
-- David

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature