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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABVgOSmNbBzR=QV4RDSdBPzBU=8mP5r0gVf5wqADm_9e9htM2g@mail.gmail.com>
Date: Thu, 21 Mar 2024 12:44:42 +0800
From: David Gow <davidgow@...gle.com>
To: Petr Tesarik <petrtesarik@...weicloud.com>
Cc: Richard Weinberger <richard@....at>, Anton Ivanov <anton.ivanov@...bridgegreys.com>, 
	Johannes Berg <johannes@...solutions.net>, 
	"open list:USER-MODE LINUX (UML)" <linux-um@...ts.infradead.org>, open list <linux-kernel@...r.kernel.org>, 
	Roberto Sassu <roberto.sassu@...weicloud.com>, 
	Petr Tesarik <petr.tesarik1@...wei-partners.com>
Subject: Re: [PATCH RESEND 1/1] um: oops on accessing a non-present page in
 the vmalloc area

On Fri, 23 Feb 2024 at 22:07, Petr Tesarik <petrtesarik@...weicloud.com> wrote:
>
> From: Petr Tesarik <petr.tesarik1@...wei-partners.com>
>
> 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@...wei-partners.com>
> ---
>  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

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4014 bytes)