linux-kernel - Re: [patch] x86: some lock annotations for user copy paths

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: <20080911082709.GA14378@elte.hu>
Date:	Thu, 11 Sep 2008 10:27:10 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Nick Piggin <npiggin@...e.de>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [patch] x86: some lock annotations for user copy paths


* Nick Piggin <npiggin@...e.de> wrote:

> On Wed, Sep 10, 2008 at 05:01:04PM +0200, Peter Zijlstra wrote:
> > >  extern struct atomic_notifier_head panic_notifier_list;
> > >  extern long (*panic_blink)(long time);
> > >  NORET_TYPE void panic(const char * fmt, ...)
> > 
> > This forgets that in_atomic() again - possibly triggering might_sleep()
> > where not appropriate.
> > 
> > I'm not sure its worth it to out-of-line the thing though (its only big
> > on debug builds), and CONFIG_LOCKDEP is the wrong CONFIG_* variable, I
> > think CONFIG_PROVE_LOCKING would be the appropriate one.
> 
> OK, last attempt. If this breaks, then I give up for the day :)

i've tidied it up a bit:

 - moved the might_sleep() check outside the in_atomic() check,
 - fixed a spelling mistake
 - fixed a build error on !LOCKDEP
 - changed the CONFIG_LOCKDEP dependency to CONFIG_PROVE_LOCKING

and it's working fine on most boxes. One testbox found this new locking 
scenario:

PM: Adding info for No Bus:vcsa7
EDAC DEBUG: MC0: i82860_check()

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.27-rc6-tip #1
-------------------------------------------------------
X/4873 is trying to acquire lock:
 (&bb->mutex){--..}, at: [<c020ba20>] mmap+0x40/0xa0

but task is already holding lock:
 (&mm->mmap_sem){----}, at: [<c0125a1e>] sys_mmap2+0x8e/0xc0

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&mm->mmap_sem){----}:
       [<c017dc96>] validate_chain+0xa96/0xf50
       [<c017ef2b>] __lock_acquire+0x2cb/0x5b0
       [<c017f299>] lock_acquire+0x89/0xc0
       [<c01aa8fb>] might_fault+0x6b/0x90
       [<c040b618>] copy_to_user+0x38/0x60
       [<c020bcfb>] read+0xfb/0x170
       [<c01c09a5>] vfs_read+0x95/0x110
       [<c01c1443>] sys_pread64+0x63/0x80
       [<c012146f>] sysenter_do_call+0x12/0x43
       [<ffffffff>] 0xffffffff

-> #0 (&bb->mutex){--..}:
       [<c017d8b7>] validate_chain+0x6b7/0xf50
       [<c017ef2b>] __lock_acquire+0x2cb/0x5b0
       [<c017f299>] lock_acquire+0x89/0xc0
       [<c0d6f2ab>] __mutex_lock_common+0xab/0x3c0
       [<c0d6f698>] mutex_lock_nested+0x38/0x50
       [<c020ba20>] mmap+0x40/0xa0
       [<c01b111e>] mmap_region+0x14e/0x450
       [<c01b170f>] do_mmap_pgoff+0x2ef/0x310
       [<c0125a3d>] sys_mmap2+0xad/0xc0
       [<c012146f>] sysenter_do_call+0x12/0x43
       [<ffffffff>] 0xffffffff

other info that might help us debug this:

1 lock held by X/4873:
 #0:  (&mm->mmap_sem){----}, at: [<c0125a1e>] sys_mmap2+0x8e/0xc0

stack backtrace:
Pid: 4873, comm: X Not tainted 2.6.27-rc6-tip #1
 [<c017cd09>] print_circular_bug_tail+0x79/0xc0
 [<c017d8b7>] validate_chain+0x6b7/0xf50
 [<c017a5b5>] ? trace_hardirqs_off_caller+0x15/0xb0
 [<c017ef2b>] __lock_acquire+0x2cb/0x5b0
 [<c017f299>] lock_acquire+0x89/0xc0
 [<c020ba20>] ? mmap+0x40/0xa0
 [<c0d6f2ab>] __mutex_lock_common+0xab/0x3c0
 [<c020ba20>] ? mmap+0x40/0xa0
 [<c0d6f698>] mutex_lock_nested+0x38/0x50
 [<c020ba20>] ? mmap+0x40/0xa0
 [<c020ba20>] mmap+0x40/0xa0
 [<c01b111e>] mmap_region+0x14e/0x450
 [<c01afb88>] ? arch_get_unmapped_area_topdown+0xf8/0x160
 [<c01b170f>] do_mmap_pgoff+0x2ef/0x310
 [<c0125a3d>] sys_mmap2+0xad/0xc0
 [<c012146f>] sysenter_do_call+0x12/0x43
 [<c0120000>] ? __switch_to+0x130/0x220
 =======================
evbug.c: Event. Dev: input3, Type: 20, Code: 0, Value: 500
warning: `sudo' uses deprecated v2 capabilities in a way that may be insecure.

i've attached the config.

at first sight it looks like a genuine bug in fs/sysfs/bin.c?

i.e. your patches are working as expected and the extended validation 
mechanism is finding real bugs :-)

	Ingo

View attachment "config" of type "text/plain" (68513 bytes)