linux-kernel - Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in gntdev

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: <20150825115211.GB1345@mail-itl>
Date:	Tue, 25 Aug 2015 13:52:11 +0200
From:	Marek Marczykowski-Górecki 
	<marmarek@...isiblethingslab.com>
To:	Luis Henriques <luis.henriques@...onical.com>
Cc:	Jiri Slaby <jslaby@...e.cz>, stable@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	David Vrabel <david.vrabel@...rix.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH 3.12 49/82] xen/gntdevt: Fix race condition in
 gntdev_release()

On Tue, Aug 25, 2015 at 12:35:59PM +0100, Luis Henriques wrote:
> [ Adding Greg has he seems to have this patch queued for 3.10 and 3.14 ]
> 
> On Mon, Aug 24, 2015 at 11:09:09AM +0200, Jiri Slaby wrote:
> > From: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>
> > 
> > 3.12-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ===============
> > 
> > commit 30b03d05e07467b8c6ec683ea96b5bffcbcd3931 upstream.
> > 
> > While gntdev_release() is called the MMU notifier is still registered
> > and can traverse priv->maps list even if no pages are mapped (which is
> > the case -- gntdev_release() is called after all). But
> > gntdev_release() will clear that list, so make sure that only one of
> > those things happens at the same time.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>
> > Signed-off-by: David Vrabel <david.vrabel@...rix.com>
> > Signed-off-by: Jiri Slaby <jslaby@...e.cz>
> > ---
> >  drivers/xen/gntdev.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> > index e41c79c986ea..f2ca8d0af55f 100644
> > --- a/drivers/xen/gntdev.c
> > +++ b/drivers/xen/gntdev.c
> > @@ -529,12 +529,14 @@ static int gntdev_release(struct inode *inode, struct file *flip)
> >  
> >  	pr_debug("priv %p\n", priv);
> >  
> > +	mutex_lock(&priv->lock);
> 
> Since 3.12 doesn't seem to include 1401c00e59ea ("xen/gntdev: convert
> priv->lock to a mutex"), this shouldn't be applied as priv->lock is
> actually a spinlock.  So, you'll need to pick 1401c00e59ea or backport
> this patch using the appropriate locking directives.  Not sure what's
> the best solution.  Maybe Marek or David can help...?

I've used spinlock approach for some time (on 3.18.x) and it works ok. This applies
also to 3.10 and 3.14 of course.

Patch here:
https://raw.githubusercontent.com/QubesOS/qubes-linux-kernel/stable-3.18/patches.xen/0001-xen-grant-fix-race-condition-in-gntdev_release.patch

and here:

From b876e14888bdafa112c3265e6420543fa74aa709 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Marek=20Marczykowski-G=C3=B3recki?=
 <marmarek@...isiblethingslab.com>
Date: Fri, 26 Jun 2015 02:16:49 +0200
Subject: [PATCH] xen/grant: fix race condition in gntdev_release
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Organization: Invisible Things Lab
Cc: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>

While gntdev_release is called, MMU notifier is still registered and
will traverse priv->maps list even if no pages are mapped (which is the
case - gntdev_release is called after all). But gntdev_release will
clear that list, so make sure that only one of those things happens at
the same time.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@...isiblethingslab.com>
---
 drivers/xen/gntdev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 8927485..4bd23bb 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -568,12 +568,14 @@ static int gntdev_release(struct inode *inode, struct file *flip)
 
 	pr_debug("priv %p\n", priv);
 
+	spin_lock(&priv->lock);
 	while (!list_empty(&priv->maps)) {
 		map = list_entry(priv->maps.next, struct grant_map, next);
 		list_del(&map->next);
 		gntdev_put_map(NULL /* already removed */, map);
 	}
 	WARN_ON(!list_empty(&priv->freeable_maps));
+	spin_unlock(&priv->lock);
 
 	if (use_ptemod)
 		mmu_notifier_unregister(&priv->mn, priv->mm);
-- 
1.9.3



-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Content of type "application/pgp-signature" skipped