linux-kernel - [PATCH] mm: get_user_pages() stores ERR

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]

Date:	Tue, 04 Aug 2009 12:39:34 +0200
From:	Brice Goglin <Brice.Goglin@...ia.fr>
To:	Leon Woestenberg <leon.woestenberg@...il.com>
CC:	Brice Goglin <Brice.Goglin@...ia.fr>,
	Hugh Dickins <hugh.dickins@...cali.co.uk>,
	linux-kernel@...r.kernel.org
Subject: [PATCH] mm: get_user_pages() stores ERR_PTR() in pages[i] on failure


>> I wonder if we should change get_user_pages to store ERR_PTR(ret)
>> in page[i] when it fails to get page #i.
>>     
> Yes, I would see that as an improvement in finding out why rc <
> nr_pages, in case rc > 0.
>
> Also I think it does not break existing users.
>   

Only compile-tested (and not in the nommu case).




When get_user_pages() fails to get a non-first page, it returns
the number of successfully gotten pages. The caller has to call
get_user_pages() again on the failed page to figure out the
error code.

Store the error code with ERR_PTR() in pages[i] when we fail
to get page #i.

The arch specific get_user_pages_fast() do not need to be changed
since they revert to the main get_user_pages() on failure.

Signed-off-by: Brice Goglin <Brice.Goglin@...ia.fr>

diff --git a/mm/memory.c b/mm/memory.c
index aede2ce..cd4efa3 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1217,6 +1217,7 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 	int force = !!(flags & GUP_FLAGS_FORCE);
 	int ignore = !!(flags & GUP_FLAGS_IGNORE_VMA_PERMISSIONS);
 	int ignore_sigkill = !!(flags & GUP_FLAGS_IGNORE_SIGKILL);
+	int err = -EFAULT;
 
 	if (nr_pages <= 0)
 		return 0;
@@ -1243,7 +1244,7 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 
 			/* user gate pages are read-only */
 			if (!ignore && write)
-				return i ? : -EFAULT;
+				goto abort;
 			if (pg > TASK_SIZE)
 				pgd = pgd_offset_k(pg);
 			else
@@ -1253,11 +1254,11 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			BUG_ON(pud_none(*pud));
 			pmd = pmd_offset(pud, pg);
 			if (pmd_none(*pmd))
-				return i ? : -EFAULT;
+				goto abort;
 			pte = pte_offset_map(pmd, pg);
 			if (pte_none(*pte)) {
 				pte_unmap(pte);
-				return i ? : -EFAULT;
+				goto abort;
 			}
 			if (pages) {
 				struct page *page = vm_normal_page(gate_vma, start, *pte);
@@ -1277,7 +1278,7 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		if (!vma ||
 		    (vma->vm_flags & (VM_IO | VM_PFNMAP)) ||
 		    (!ignore && !(vm_flags & vma->vm_flags)))
-			return i ? : -EFAULT;
+			goto abort;
 
 		if (is_vm_hugetlb_page(vma)) {
 			i = follow_hugetlb_page(mm, vma, pages, vmas,
@@ -1302,8 +1303,10 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 			 * we're only unlocking already resident/mapped pages.
 			 */
 			if (unlikely(!ignore_sigkill &&
-					fatal_signal_pending(current)))
-				return i ? i : -ERESTARTSYS;
+					fatal_signal_pending(current))) {
+				err = -ERESTARTSYS;
+				goto abort;
+			}
 
 			if (write)
 				foll_flags |= FOLL_WRITE;
@@ -1317,10 +1320,11 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 					FAULT_FLAG_WRITE : 0);
 
 				if (ret & VM_FAULT_ERROR) {
-					if (ret & VM_FAULT_OOM)
-						return i ? i : -ENOMEM;
-					else if (ret & VM_FAULT_SIGBUS)
-						return i ? i : -EFAULT;
+					if (ret & VM_FAULT_OOM) {
+						err = -ENOMEM;
+						goto abort;
+					} else if (ret & VM_FAULT_SIGBUS)
+						goto abort;
 					BUG();
 				}
 				if (ret & VM_FAULT_MAJOR)
@@ -1346,8 +1350,10 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 
 				cond_resched();
 			}
-			if (IS_ERR(page))
-				return i ? i : PTR_ERR(page);
+			if (IS_ERR(page)) {
+				err = PTR_ERR(page);
+				goto abort;
+			}
 			if (pages) {
 				pages[i] = page;
 
@@ -1362,6 +1368,11 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		} while (nr_pages && start < vma->vm_end);
 	} while (nr_pages);
 	return i;
+
+ abort:
+	if (pages)
+		pages[i] = ERR_PTR(err);
+	return i ? : err;
 }
 
 /**
diff --git a/mm/nommu.c b/mm/nommu.c
index 53cab10..5fe8083 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -212,6 +212,8 @@ int __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 	return i;
 
 finish_or_fault:
+	if (pages)
+		pages[i] = ERR_PTR(-EFAULT);
 	return i ? : -EFAULT;
 }
 


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