linux-kernel - Re: [RFC][PATCH v3 6/6] fix wrong get_user

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: <20090419202447.FFC2.A69D9226@jp.fujitsu.com>
Date:	Sun, 19 Apr 2009 21:37:35 +0900 (JST)
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	"Sosnowski, Maciej" <maciej.sosnowski@...el.com>
Cc:	kosaki.motohiro@...fujitsu.com,
	Nick Piggin <nickpiggin@...oo.com.au>,
	LKML <linux-kernel@...r.kernel.org>,
	Linus Torvalds <torvalds@...l.org>,
	Andrew Morton <akpm@...l.org>,
	Andrea Arcangeli <aarcange@...hat.com>,
	Jeff Moyer <jmoyer@...hat.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>,
	"Leech, Christopher" <christopher.leech@...el.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [RFC][PATCH v3 6/6] fix wrong get_user_pages usage in iovlock.c

> KOSAKI Motohiro wrote:
> >>> I would perhaps not fold gup_fast conversions into the same patch as
> >>> the fix.
> >> 
> >> OK. I'll fix.
> > 
> > Done.
> > 
> > 
> > 
> > ===================================
> > Subject: [Untested][RFC][PATCH] fix wrong get_user_pages usage in iovlock.c
> > 
> > 	down_read(mmap_sem)
> > 	get_user_pages()
> > 	up_read(mmap_sem)
> > 
> > is fork unsafe.
> > fix it.
> > 
> > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
> > Cc: Maciej Sosnowski <maciej.sosnowski@...el.com>
> > Cc: David S. Miller <davem@...emloft.net>
> > Cc: Chris Leech <christopher.leech@...el.com>
> > Cc: netdev@...r.kernel.org
> > ---
> >  drivers/dma/iovlock.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > Index: b/drivers/dma/iovlock.c
> > ===================================================================
> > --- a/drivers/dma/iovlock.c	2009-04-13 22:58:36.000000000 +0900
> > +++ b/drivers/dma/iovlock.c	2009-04-14 20:27:16.000000000 +0900
> > @@ -104,8 +104,6 @@ struct dma_pinned_list *dma_pin_iovec_pa
> >  			0,	/* force */
> >  			page_list->pages,
> >  			NULL);
> > -		up_read(&current->mm->mmap_sem);
> > -
> >  		if (ret != page_list->nr_pages)
> >  			goto unpin;
> > 
> > @@ -127,6 +125,8 @@ void dma_unpin_iovec_pages(struct dma_pi
> >  	if (!pinned_list)
> >  		return;
> > 
> > +	up_read(&current->mm->mmap_sem);
> > +
> >  	for (i = 0; i < pinned_list->nr_iovecs; i++) {
> >  		struct dma_page_list *page_list = &pinned_list->page_list[i];
> >  		for (j = 0; j < page_list->nr_pages; j++) {
> 
> I have tried it with net_dma and here is what I've got.

Thanks.
Instead, How about this?


============================================
Subject: [Untested][RFC][PATCH v3] fix wrong get_user_pages usage in iovlock.c

	down_read(mmap_sem)
	get_user_pages()
	up_read(mmap_sem)

is fork unsafe.
mmap_sem should't be released until dma_unpin_iovec_pages() is called.


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
Cc: Maciej Sosnowski <maciej.sosnowski@...el.com>
Cc: David S. Miller <davem@...emloft.net>
Cc: Chris Leech <christopher.leech@...el.com>
Cc: netdev@...r.kernel.org
---
 drivers/dma/iovlock.c |    5 ++---
 net/ipv4/tcp.c        |    9 +++++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

Index: b/drivers/dma/iovlock.c
===================================================================
--- a/drivers/dma/iovlock.c	2009-04-19 17:27:25.000000000 +0900
+++ b/drivers/dma/iovlock.c	2009-04-19 17:29:42.000000000 +0900
@@ -45,6 +45,8 @@ static int num_pages_spanned(struct iove
  * We are allocating a single chunk of memory, and then carving it up into
  * 3 sections, the latter 2 whose size depends on the number of iovecs and the
  * total number of pages, respectively.
+ *
+ * Caller must hold mm->mmap_sem
  */
 struct dma_pinned_list *dma_pin_iovec_pages(struct iovec *iov, size_t len)
 {
@@ -94,7 +96,6 @@ struct dma_pinned_list *dma_pin_iovec_pa
 		pages += page_list->nr_pages;
 
 		/* pin pages down */
-		down_read(&current->mm->mmap_sem);
 		ret = get_user_pages(
 			current,
 			current->mm,
@@ -104,8 +105,6 @@ struct dma_pinned_list *dma_pin_iovec_pa
 			0,	/* force */
 			page_list->pages,
 			NULL);
-		up_read(&current->mm->mmap_sem);
-
 		if (ret != page_list->nr_pages)
 			goto unpin;
 
Index: b/net/ipv4/tcp.c
===================================================================
--- a/net/ipv4/tcp.c	2009-04-19 17:27:25.000000000 +0900
+++ b/net/ipv4/tcp.c	2009-04-19 18:09:42.000000000 +0900
@@ -1322,6 +1322,9 @@ int tcp_recvmsg(struct kiocb *iocb, stru
 	int copied_early = 0;
 	struct sk_buff *skb;
 
+#ifdef CONFIG_NET_DMA
+	down_read(&current->mm->mmap_sem);
+#endif
 	lock_sock(sk);
 
 	TCP_CHECK_TIMER(sk);
@@ -1688,11 +1691,17 @@ skip_copy:
 
 	TCP_CHECK_TIMER(sk);
 	release_sock(sk);
+#ifdef CONFIG_NET_DMA
+	up_read(&current->mm->mmap_sem);
+#endif
 	return copied;
 
 out:
 	TCP_CHECK_TIMER(sk);
 	release_sock(sk);
+#ifdef CONFIG_NET_DMA
+	up_read(&current->mm->mmap_sem);
+#endif
 	return err;
 
 recv_urg:


--
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/