linux-kernel - Re: [PATCH -tip] kmemcheck: fix dynamic enable/disable

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: <1227530628.3718.122.camel@penberg-laptop>
Date:	Mon, 24 Nov 2008 14:43:48 +0200
From:	Pekka Enberg <penberg@...helsinki.fi>
To:	Vegard Nossum <vegard.nossum@...il.com>
Cc:	Sitsofe Wheeler <sitsofe@...oo.com>, linux-kernel@...r.kernel.org,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH -tip] kmemcheck: fix dynamic enable/disable

Hi Vegard,

On Mon, 2008-11-24 at 13:20 +0100, Vegard Nossum wrote:
> (Ingo: Will send pull request for this later :-))
> 
> 
> From 6f505f59011e565c2dbb7a220702feb0447cc854 Mon Sep 17 00:00:00 2001
> From: Vegard Nossum <vegard.nossum@...il.com>
> Date: Mon, 24 Nov 2008 11:52:05 +0100
> Subject: [PATCH] kmemcheck: fix dynamic enable/disable
> 
> On Fri, Oct 10, 2008 at 1:40 PM, Sitsofe Wheeler <sitsofe@...oo.com> wrote:
> > I received a lot of errors after manually enabling kmemcheck
> > in /proc (it had been turned off due to oneshot mode). It
> > might be a good idea to put a warning that errrors found after
> > enabling it at any time other than boot might be spurious.
> 
> Fixed. This was the problem: Instructions with multiple address
> operands could look up the shadow of an address that had
> previously been un-hidden. After handling the memory access, the
> page would get hidden again. The fix is to verify that the page
> is present before we return a shadow pointer.

I don't quite understand the error condition nor the fix. It's probably
because I fried my brain during the weekend but here goes...

> diff --git a/arch/x86/mm/kmemcheck/shadow.c b/arch/x86/mm/kmemcheck/shadow.c
> index 196dddc..62a0f63 100644
> --- a/arch/x86/mm/kmemcheck/shadow.c
> +++ b/arch/x86/mm/kmemcheck/shadow.c
> @@ -27,6 +27,8 @@ void *kmemcheck_shadow_lookup(unsigned long address)
>  	pte = kmemcheck_pte_lookup(address);
>  	if (!pte)
>  		return NULL;
> +	if (pte_present(*pte))
> +		return NULL;

OK, so when kmemcheck is disabled in oneshot mode they're made present
and we don't mark them as non-present when we enable kmemcheck again? Is
there a reason we can't do that? You might want add a comment explaining
what's going on here.
 
>  	page = virt_to_page(address);
>  	if (!page->shadow)

> diff --git a/arch/x86/mm/kmemcheck/kmemcheck.c b/arch/x86/mm/kmemcheck/kmemcheck.c
> index 056b4f1..12a4bbd 100644
> --- a/arch/x86/mm/kmemcheck/kmemcheck.c
> +++ b/arch/x86/mm/kmemcheck/kmemcheck.c
> @@ -707,6 +707,16 @@ bool kmemcheck_trap(struct pt_regs *regs)
>  	}
>  
>  	/*
> +	 * At this point, we know that the trap was kmemcheck's. However, if
> +	 * kmemcheck was disabled, we need to return immediately (and stop
> +	 * emulating the REP instruction).
> +	 */
> +	if (!kmemcheck_enabled) {
> +		kmemcheck_hide(regs);
> +		return true;
> +	}

OK, I assume kmemcheck_hide() stops emulating the REP instruction? Why
do we need to do it here and not at kmemcheck disable-time? Also, this
change doesn't seem to be explained in the patch description?

		Pekka

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/