Remove easily user-triggerable BUG from generic_setlease

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

index 814c51d..fce6238 100644
@@ -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);

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

Yeah, box wedged shortly afterwards. Not cool.

yeah, definitly. my bad.

Dave

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 <[email protected]>
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.

index 43797a9..ad1de47 100644
@@ -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;

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

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.

Does it do anything in mainline?

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

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.)

---
 fs/locks.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

OK, makes sense. Resending the patch to make it clear it's intended for mainline as well.--b.

index 43797a9..ad1de47 100644
@@ -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