* Remove easily user-triggerable BUG from generic_setlease @ 2012-07-13 17:35 Dave Jones 2012-07-13 17:47 ` Linus Torvalds 0 siblings, 1 reply; 8+ messages in thread From: Dave Jones @ 2012-07-13 17:35 UTC (permalink / raw) To: Linux Kernel; +Cc: J. Bruce Fields, Linus Torvalds This can be trivially triggered from userspace by passing in something unexpected. [126749.760961] kernel BUG at fs/locks.c:1468! [126749.761849] invalid opcode: 0000 [#1] SMP [126749.762490] CPU 2 [126749.811520] Pid: 15891, comm: trinity-child2 Not tainted 3.5.0-rc6+ #105 [126749.813723] RIP: 0010:[<ffffffff81222802>] [<ffffffff81222802>] generic_setlease+0xc2/0x100 [126749.814823] RSP: 0018:ffff88011a979e88 EFLAGS: 00010286 [126749.815899] RAX: ffffffff81822b40 RBX: ffff8800912e0040 RCX: 0000000000000001 [126749.816970] RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff8800912e0040 [126749.818037] RBP: ffff88011a979eb8 R08: 0000000000000001 R09: fffffffff7380232 [126749.819098] R10: ffffffff82210560 R11: 0000000000000232 R12: ffff88011c879b80 [126749.820149] R13: ffffffff00000000 R14: ffff88011a979ee0 R15: 00000000000003e8 [126749.821189] FS: 00007fb7d1340740(0000) GS:ffff880148000000(0000) knlGS:0000000000000000 [126749.822222] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [126749.823251] CR2: 00000000029a8000 CR3: 0000000119fd0000 CR4: 00000000001407e0 [126749.824278] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [126749.825287] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [126749.826274] Process trinity-child2 (pid: 15891, threadinfo ffff88011a978000, task ffff88000abf2690) [126749.827260] Stack: [126749.828225] ffff88011a979eb8 ffffffff00000000 ffff88011c879b80 ffff8800085326c8 [126749.829202] ffff880107f85810 00000000000000b1 ffff88011a979ec8 ffffffff81222875 [126749.830168] ffff88011a979f18 ffffffff81222a06 ffff88011a979f18 ffff8800085326c8 [126749.831123] Call Trace: [126749.832054] [<ffffffff81222875>] __vfs_setlease+0x35/0x40 [126749.832979] [<ffffffff81222a06>] fcntl_setlease+0x76/0x150 [126749.833905] [<ffffffff811e1876>] sys_fcntl+0x1c6/0x810 [126749.834800] [<ffffffff8134b4ae>] ? trace_hardirqs_on_thunk+0x3a/0x3f [126749.835687] [<ffffffff81691d2d>] system_call_fastpath+0x1a/0x1f Signed-off-by: Dave Jones <davej@redhat.com> diff --git a/fs/locks.c b/fs/locks.c index 814c51d..fce6238 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -1465,7 +1465,7 @@ int generic_setlease(struct file *filp, long arg, struct file_lock **flp) case F_WRLCK: return generic_add_lease(filp, arg, flp); default: - BUG(); + return -EINVAL; } } EXPORT_SYMBOL(generic_setlease); ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Remove easily user-triggerable BUG from generic_setlease 2012-07-13 17:35 Remove easily user-triggerable BUG from generic_setlease Dave Jones @ 2012-07-13 17:47 ` Linus Torvalds 2012-07-13 17:50 ` Dave Jones 2012-07-23 15:20 ` J. Bruce Fields 0 siblings, 2 replies; 8+ messages in thread From: Linus Torvalds @ 2012-07-13 17:47 UTC (permalink / raw) To: Dave Jones, Linux Kernel, J. Bruce Fields, Linus Torvalds On Fri, Jul 13, 2012 at 10:35 AM, Dave Jones <davej@redhat.com> wrote: > This can be trivially triggered from userspace by passing in something unexpected. Argh. It looks like it would be harmless (apart from the noise), except we hold file_lock_lock. Which turns the BUG_ON() into not just "noise and kill the process", but "noise and kill the process and leave a nasty lock held". This seems to go back to 3.2, so stable should be cc'd, no? Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Remove easily user-triggerable BUG from generic_setlease 2012-07-13 17:47 ` Linus Torvalds @ 2012-07-13 17:50 ` Dave Jones 2012-07-23 15:20 ` J. Bruce Fields 1 sibling, 0 replies; 8+ messages in thread From: Dave Jones @ 2012-07-13 17:50 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel, J. Bruce Fields On Fri, Jul 13, 2012 at 10:47:43AM -0700, Linus Torvalds wrote: > On Fri, Jul 13, 2012 at 10:35 AM, Dave Jones <davej@redhat.com> wrote: > > This can be trivially triggered from userspace by passing in something unexpected. > > Argh. It looks like it would be harmless (apart from the noise), > except we hold file_lock_lock. Which turns the BUG_ON() into not just > "noise and kill the process", but "noise and kill the process and > leave a nasty lock held". Yeah, box wedged shortly afterwards. Not cool. > This seems to go back to 3.2, so stable should be cc'd, no? yeah, definitly. my bad. Dave ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Remove easily user-triggerable BUG from generic_setlease 2012-07-13 17:47 ` Linus Torvalds 2012-07-13 17:50 ` Dave Jones @ 2012-07-23 15:20 ` J. Bruce Fields 2012-07-23 18:34 ` Linus Torvalds 1 sibling, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2012-07-23 15:20 UTC (permalink / raw) To: Linus Torvalds; +Cc: Dave Jones, Linux Kernel, J. Bruce Fields On Fri, Jul 13, 2012 at 10:47:43AM -0700, Linus Torvalds wrote: > On Fri, Jul 13, 2012 at 10:35 AM, Dave Jones <davej@redhat.com> wrote: > > This can be trivially triggered from userspace by passing in something unexpected. > > Argh. It looks like it would be harmless (apart from the noise), > except we hold file_lock_lock. Which turns the BUG_ON() into not just > "noise and kill the process", but "noise and kill the process and > leave a nasty lock held". > > This seems to go back to 3.2, so stable should be cc'd, no? Thanks! Yes, this fixes the bug for >=3.2, but before the addition of this BUG() we could get memory corruption in this case. And that problem existed since the original introduction of the lease code, as far as I can tell. So we need something like the following, backported to 2.6.anything. --b. commit 76fca57d7f4e408fc758a42f798c2ebef54be60f Author: J. Bruce Fields <bfields@redhat.com> Date: Wed Jul 18 17:45:42 2012 -0600 locks: fix checking of fcntl_setlease argument The only checks of the long argument passed to fcntl(fd,F_SETLEASE,.) are done after converting the long to an int. Thus some illegal values may be let through and cause problems in later code. Cc: stable@vger.kernel.org Signed-off-by: J. Bruce Fields <bfields@redhat.com> diff --git a/fs/locks.c b/fs/locks.c index 43797a9..ad1de47 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -311,7 +311,7 @@ static int flock_make_lock(struct file *filp, struct file_lock **lock, return 0; } -static int assign_type(struct file_lock *fl, int type) +static int assign_type(struct file_lock *fl, long type) { switch (type) { case F_RDLCK: @@ -448,7 +448,7 @@ static const struct lock_manager_operations lease_manager_ops = { /* * Initialize a lease, use the default lock manager operations */ -static int lease_init(struct file *filp, int type, struct file_lock *fl) +static int lease_init(struct file *filp, long type, struct file_lock *fl) { if (assign_type(fl, type) != 0) return -EINVAL; @@ -466,7 +466,7 @@ static int lease_init(struct file *filp, int type, struct file_lock *fl) } /* Allocate a file_lock initialised to this type of lease */ -static struct file_lock *lease_alloc(struct file *filp, int type) +static struct file_lock *lease_alloc(struct file *filp, long type) { struct file_lock *fl = locks_alloc_lock(); int error = -ENOMEM; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: Remove easily user-triggerable BUG from generic_setlease 2012-07-23 15:20 ` J. Bruce Fields @ 2012-07-23 18:34 ` Linus Torvalds 2012-07-23 19:04 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2012-07-23 18:34 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Dave Jones, Linux Kernel, J. Bruce Fields On Mon, Jul 23, 2012 at 8:20 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > So we need something like the following, backported to 2.6.anything. Please add a note about the 3.2+ version of this patch (well, totally different patch), and why this particular patch isn't needed there. For stable, we should always have a pointer to the patch in mainline, and if mainline has a different solution, and note about *why* mainline has that different solution. Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Remove easily user-triggerable BUG from generic_setlease 2012-07-23 18:34 ` Linus Torvalds @ 2012-07-23 19:04 ` J. Bruce Fields 2012-07-23 19:09 ` Linus Torvalds 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2012-07-23 19:04 UTC (permalink / raw) To: Linus Torvalds; +Cc: Dave Jones, Linux Kernel, J. Bruce Fields On Mon, Jul 23, 2012 at 11:34:56AM -0700, Linus Torvalds wrote: > On Mon, Jul 23, 2012 at 8:20 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > So we need something like the following, backported to 2.6.anything. > > Please add a note about the 3.2+ version of this patch (well, totally > different patch), and why this particular patch isn't needed there. > > For stable, we should always have a pointer to the patch in mainline, > and if mainline has a different solution, and note about *why* > mainline has that different solution. Right, I wasn't clear: that patch should go to mainline as well. (Then, do we still want Dave's patch?: in some sense that BUG() was correct, as the code was obviously intended to catch illegal values earlier. And having the BUG() means we found the problem quickly instead of having to track down memory corruption. On the other hand, agreed that BUG()'ing under a spin lock is cruel. Maybe we should stick a WARN there if it's not overkill.) --b. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: Remove easily user-triggerable BUG from generic_setlease 2012-07-23 19:04 ` J. Bruce Fields @ 2012-07-23 19:09 ` Linus Torvalds 2012-07-23 19:17 ` [PATCH] locks: fix checking of fcntl_setlease argument J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Linus Torvalds @ 2012-07-23 19:09 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Dave Jones, Linux Kernel, J. Bruce Fields On Mon, Jul 23, 2012 at 12:04 PM, J. Bruce Fields <bfields@fieldses.org> wrote: > > Right, I wasn't clear: that patch should go to mainline as well. Does it do anything in mainline? > (Then, do we still want Dave's patch?: in some sense that BUG() was > correct Hell no. A BUG() is *never* correct unless it's a situation where not having the bug would do something worse (ie subtly corrupt memory). And quite frankly, if you had a BUG() there and knew about the memory corruption, that's just a f*cking disgrace. So no, no excuse for BUG()s like that. NEVER EVER add BUG() as a "well, that was unexpected". That way lies exactly the kinds of denial-of-service attacks that that BUG() caused. The only valid source of BUG() is if you actually find internal data structure *corruption*, and you say "ok, I cannot possibly continue, because anything I would do would be wrong". Seriously. People who use BUG() statements like some kind of assert() are a menace to society. They kill machines. Linus ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] locks: fix checking of fcntl_setlease argument 2012-07-23 19:09 ` Linus Torvalds @ 2012-07-23 19:17 ` J. Bruce Fields 0 siblings, 0 replies; 8+ messages in thread From: J. Bruce Fields @ 2012-07-23 19:17 UTC (permalink / raw) To: Linus Torvalds; +Cc: Dave Jones, Linux Kernel, J. Bruce Fields The only checks of the long argument passed to fcntl(fd,F_SETLEASE,.) are done after converting the long to an int. Thus some illegal values may be let through and cause problems in later code. (They actually *don't* cause problems in mainline, as of Dave Jones's 8d657eb3b43861064d36241e88d9d61c709f33f0 "Remove easily user-triggerable BUG from generic_setlease", but we should fix this anyway. And this patch will be necessary to fix real bugs on earlier kernels.) Cc: stable@vger.kernel.org Signed-off-by: J. Bruce Fields <bfields@redhat.com> --- fs/locks.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) On Mon, Jul 23, 2012 at 12:09:21PM -0700, Linus Torvalds wrote: > NEVER EVER add BUG() as a "well, that was unexpected". That way lies > exactly the kinds of denial-of-service attacks that that BUG() caused. OK, makes sense. Resending the patch to make it clear it's intended for mainline as well.--b. diff --git a/fs/locks.c b/fs/locks.c index 43797a9..ad1de47 100644 --- a/fs/locks.c +++ b/fs/locks.c @@ -311,7 +311,7 @@ static int flock_make_lock(struct file *filp, struct file_lock **lock, return 0; } -static int assign_type(struct file_lock *fl, int type) +static int assign_type(struct file_lock *fl, long type) { switch (type) { case F_RDLCK: @@ -448,7 +448,7 @@ static const struct lock_manager_operations lease_manager_ops = { /* * Initialize a lease, use the default lock manager operations */ -static int lease_init(struct file *filp, int type, struct file_lock *fl) +static int lease_init(struct file *filp, long type, struct file_lock *fl) { if (assign_type(fl, type) != 0) return -EINVAL; @@ -466,7 +466,7 @@ static int lease_init(struct file *filp, int type, struct file_lock *fl) } /* Allocate a file_lock initialised to this type of lease */ -static struct file_lock *lease_alloc(struct file *filp, int type) +static struct file_lock *lease_alloc(struct file *filp, long type) { struct file_lock *fl = locks_alloc_lock(); int error = -ENOMEM; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-07-23 19:17 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-07-13 17:35 Remove easily user-triggerable BUG from generic_setlease Dave Jones 2012-07-13 17:47 ` Linus Torvalds 2012-07-13 17:50 ` Dave Jones 2012-07-23 15:20 ` J. Bruce Fields 2012-07-23 18:34 ` Linus Torvalds 2012-07-23 19:04 ` J. Bruce Fields 2012-07-23 19:09 ` Linus Torvalds 2012-07-23 19:17 ` [PATCH] locks: fix checking of fcntl_setlease argument J. Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.