diff mbox series

[07/35] mm/gup: fix null pointer dereference detected by coverity

Message ID 20200410213235.M6eTaELL2%akpm@linux-foundation.org (mailing list archive)
State New, archived
Headers show
Series [01/35] hfsplus: fix crash and filesystem corruption when deleting files | expand

Commit Message

Andrew Morton April 10, 2020, 9:32 p.m. UTC 
From: Miles Chen <miles.chen@mediatek.com>
Subject: mm/gup: fix null pointer dereference detected by coverity

In fixup_user_fault(), it is possible that unlocked is NULL,
so we should test unlocked before using it.

For example, in arch/arc/kernel/process.c, NULL is passed
to fixup_user_fault().

SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new)
{
...
	ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr,
			       FAULT_FLAG_WRITE, NULL);
...
}

Link: http://lkml.kernel.org/r/20200407095107.1988-1-miles.chen@mediatek.com
Fixes: 4a9e1cda2748 ("mm: bring in additional flag for fixup_user_fault to signal unlock")
Signed-off-by: Miles Chen <miles.chen@mediatek.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/gup.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Linus Torvalds April 10, 2020, 10:24 p.m. UTC | #1 
On Fri, Apr 10, 2020 at 2:32 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> In fixup_user_fault(), it is possible that unlocked is NULL,
> so we should test unlocked before using it.

This seems wrong.

> For example, in arch/arc/kernel/process.c, NULL is passed
> to fixup_user_fault().
>
>         ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr,
>                                FAULT_FLAG_WRITE, NULL);

Yes, but it doesn't set FAULT_FLAG_ALLOW_RETRY, exactly _because_
'unlocked' is NULL.

Basically, retry is fundamentally tied to that "unlocked" flag. You
can't ask for retry without also saying "please tell me if you
unlocked the mmap_sem during the retry". So the two go hand in hand
there.

So I think this is just coverity not understanding the rules.

Or maybe I'm the one missing something. Did you actually see a problem?

                   Linus
Peter Xu April 10, 2020, 11:53 p.m. UTC | #2 
On Fri, Apr 10, 2020 at 03:24:37PM -0700, Linus Torvalds wrote:
> On Fri, Apr 10, 2020 at 2:32 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > In fixup_user_fault(), it is possible that unlocked is NULL,
> > so we should test unlocked before using it.
> 
> This seems wrong.
> 
> > For example, in arch/arc/kernel/process.c, NULL is passed
> > to fixup_user_fault().
> >
> >         ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr,
> >                                FAULT_FLAG_WRITE, NULL);
> 
> Yes, but it doesn't set FAULT_FLAG_ALLOW_RETRY, exactly _because_
> 'unlocked' is NULL.
> 
> Basically, retry is fundamentally tied to that "unlocked" flag. You
> can't ask for retry without also saying "please tell me if you
> unlocked the mmap_sem during the retry". So the two go hand in hand
> there.
> 
> So I think this is just coverity not understanding the rules.

Yeah that's also my understanding - VM_FAULT_RETRY should depend on
FAULT_FLAG_ALLOW_RETRY, which further depends on unlocked != NULL.

Though I'm not sure how to teach Coverity about the fact.  Maybe a
"BUG_ON(unlocked == NULL)" (which contains an unreachable() inside)
before referencing unlocked?

Thanks,

> 
> Or maybe I'm the one missing something. Did you actually see a problem?
> 
>                    Linus
>
Linus Torvalds April 11, 2020, 12:19 a.m. UTC | #3 
On Fri, Apr 10, 2020 at 4:54 PM Peter Xu <peterx@redhat.com> wrote:
>
> Though I'm not sure how to teach Coverity about the fact.  Maybe a
> "BUG_ON(unlocked == NULL)" (which contains an unreachable() inside)
> before referencing unlocked?

Please don't add BUG_ON's.

In the "that would be a horrible bug" case, I'd rather see a NULL
pointer cause a fault, than see a BUG_ON.

                 Linus
Miles Chen April 14, 2020, 4:04 a.m. UTC | #4 
On Fri, 2020-04-10 at 15:24 -0700, Linus Torvalds wrote:
> On Fri, Apr 10, 2020 at 2:32 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > In fixup_user_fault(), it is possible that unlocked is NULL,
> > so we should test unlocked before using it.
> 
> This seems wrong.
> 
> > For example, in arch/arc/kernel/process.c, NULL is passed
> > to fixup_user_fault().
> >
> >         ret = fixup_user_fault(current, current->mm, (unsigned long) uaddr,
> >                                FAULT_FLAG_WRITE, NULL);
> 
> Yes, but it doesn't set FAULT_FLAG_ALLOW_RETRY, exactly _because_
> 'unlocked' is NULL.
> 
> Basically, retry is fundamentally tied to that "unlocked" flag. You
> can't ask for retry without also saying "please tell me if you
> unlocked the mmap_sem during the retry". So the two go hand in hand
> there.
> 
> So I think this is just coverity not understanding the rules.
> 
> Or maybe I'm the one missing something. Did you actually see a problem?

Thanks for the explanation, it is just a coverity issue, not a real
problem.

I worry about the following case: someone passes FAULT_FLAG_ALLOW_RETRY
to fixup_user_fault() with unlocked == NULL.

e.g., 
ret = fixup_user_fault(current, current->mm, uaddr,
                       flags | FAULT_FLAG_ALLOW_RETRY, NULL);


#define FAULT_FLAG_DEFAULT  (FAULT_FLAG_ALLOW_RETRY | \
                             FAULT_FLAG_KILLABLE | \
                             FAULT_FLAG_INTERRUPTIBLE)


In fixup_user_fault(), it adds FAULT_FLAG_ALLOW_RETRY if unlocked is not
NULL, but it does not remove FAULT_FLAG_ALLOW_RETRY if unlocked is NULL.

int fixup_user_fault(struct task_struct *tsk, struct mm_struct *mm,
                     unsigned long address, unsigned int fault_flags,
                     bool *unlocked)
{
...
	if (unlocked)
		fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
...
}

Things could go wrong in the above case.

I checked the source code and there is no such case. I worry too much
about it.

  Miles
> 
>                    Linus
diff mbox series

Patch

--- a/mm/gup.c~mm-gup-fix-null-pointer-dereference-detected-by-coverity
+++ a/mm/gup.c
@@ -1231,7 +1231,8 @@  retry:
 	if (ret & VM_FAULT_RETRY) {
 		down_read(&mm->mmap_sem);
 		if (!(fault_flags & FAULT_FLAG_TRIED)) {
-			*unlocked = true;
+			if (unlocked)
+				*unlocked = true;
 			fault_flags |= FAULT_FLAG_TRIED;
 			goto retry;
 		}