linux-kernel - Re: [PATCH] kmmio/mmiotrace: fix double free of kmmio_fault

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: <20100605202933.7a650c62@daedalus.pq.iki.fi>
Date:	Sat, 5 Jun 2010 20:29:33 +0300
From:	Pekka Paalanen <pq@....fi>
To:	Marcin Slusarz <marcin.slusarz@...il.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Stuart Bennett <stuart@...edesktop.org>,
	Christoph Bumiller <e0425955@...dent.tuwien.ac.at>,
	Shinpei KATO <shinpei@...is.s.u-tokyo.ac.jp>,
	nouveau@...ts.freedesktop.org, x86@...nel.org
Subject: Re: [PATCH] kmmio/mmiotrace: fix double free of kmmio_fault_pages

On Sat, 5 Jun 2010 18:49:42 +0200
Marcin Slusarz <marcin.slusarz@...il.com> wrote:

> After every iounmap mmiotrace has to free kmmio_fault_pages, but
> it can't do it directly, so it defers freeing by RCU.
> 
> It usually works, but when mmiotraced code calls ioremap-iounmap
> multiple times without sleeping between (so RCU won't kick in and
> start freeing) it can be given the same virtual address, so at
> every iounmap mmiotrace will schedule the same pages for release.
> Obviously it will explode on second free.
> 
> Fix it by marking kmmio_fault_pages which are scheduled for
> release and not adding them second time.
> 
> Signed-off-by: Marcin Slusarz <marcin.slusarz@...il.com>
> Cc: Pekka Paalanen <pq@....fi>
> Cc: Stuart Bennett <stuart@...edesktop.org>

Excellent work! Unfortunately I cannot review this patch
right now, I am sick. The description sounds good, though,
and I have no objections.


Thank you very much!

> ---
>  arch/x86/mm/kmmio.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/mm/kmmio.c b/arch/x86/mm/kmmio.c
> index 5d0e67f..e5d5e2c 100644
> --- a/arch/x86/mm/kmmio.c
> +++ b/arch/x86/mm/kmmio.c
> @@ -45,6 +45,8 @@ struct kmmio_fault_page {
>  	 * Protected by kmmio_lock, when linked into
> kmmio_page_table. */
>  	int count;
> +
> +	bool scheduled_for_release;
>  };
>  
>  struct kmmio_delayed_release {
> @@ -398,8 +400,11 @@ static void
> release_kmmio_fault_page(unsigned long page, BUG_ON(f->count < 0);
>  	if (!f->count) {
>  		disarm_kmmio_fault_page(f);
> -		f->release_next = *release_list;
> -		*release_list = f;
> +		if (!f->scheduled_for_release) {
> +			f->release_next = *release_list;
> +			*release_list = f;
> +			f->scheduled_for_release = true;
> +		}
>  	}
>  }
>  
> @@ -471,8 +476,10 @@ static void remove_kmmio_fault_pages(struct
> rcu_head *head) prevp = &f->release_next;
>  		} else {
>  			*prevp = f->release_next;
> +			f->release_next = NULL;
> +			f->scheduled_for_release = false;
>  		}
> -		f = f->release_next;
> +		f = *prevp;
>  	}
>  	spin_unlock_irqrestore(&kmmio_lock, flags);
>  
> @@ -510,6 +517,9 @@ void unregister_kmmio_probe(struct
> kmmio_probe *p) kmmio_count--;
>  	spin_unlock_irqrestore(&kmmio_lock, flags);
>  
> +	if (!release_list)
> +		return;
> +
>  	drelease = kmalloc(sizeof(*drelease), GFP_ATOMIC);
>  	if (!drelease) {
>  		pr_crit("leaking kmmio_fault_page objects.\n");
> -- 
> 1.7.1
> 
> 


-- 
Pekka Paalanen
http://www.iki.fi/pq/
--
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/