Remove easily user-triggerable BUG from generic_setlease

Remove easily user-triggerable BUG from generic_setlease

[Dave Jones]

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

Re: Remove easily user-triggerable BUG from generic_setlease

[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

Re: Remove easily user-triggerable BUG from generic_setlease

[Dave Jones]

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

Re: Remove easily user-triggerable BUG from generic_setlease

[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;

Re: Remove easily user-triggerable BUG from generic_setlease

[Linus Torvalds]

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

Re: Remove easily user-triggerable BUG from generic_setlease

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

Re: Remove easily user-triggerable BUG from generic_setlease

[Linus Torvalds]

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

[PATCH] locks: fix checking of fcntl_setlease argument

[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