Re: Loop encryption module locking bug (linux-2.4.5).

Re: Loop encryption module locking bug (linux-2.4.5).

[Andi Kleen]

<sarcasm>
I think your mail is offtopic for linux-kernel: it doesn't mention Microsoft or user space
java programming or pointer to random unrelated web pages, but an actual kernel bug.
</sarcasm> 

Ingo Rohloff <rohloff@in.tum.de> writes:
> 
> If lo_open doesn't call the cipher lock function and 
> lo_release doesn't call the cipher unlock function, the issue
> is resolved. (So code gets deleted in the patch.)

I think it would be better if the low level module stays locked also while the 
control fd is open. That would match the semantics of most other devices.

Right fix probably is to call ->lock twice in loop_set_status()

[Also the locking is not SMP safe, but that's a different issue, for the e.g. ->lock
would need to be replaced with a struct module *owner and also some other locking]

-Andi

Re: Loop encryption module locking bug (linux-2.4.5).

[Ingo Rohloff]

> > Locking twice? But what happens if some program calls loop_set_status more
> > than once? Losetup doesn't, but if such program exists, locking is still
> > screwed.
> 
> No, it calls loop_release_xfer always before init_xfer, which will release
> the "permanent" use count.
Calling lock twice in loop_set_status is not good, as long as you
don't call unlock twice too (in case that you change the transfer function):

losetup -e twofish /dev/loop0 file1 results in:

loop_open       // lock not called, because no cipher selected: lock 0
loop_set_fd     // has first to be bounded to file1
loop_set_status // lock called 2 times: lock 2
loop_release    // unlock called: lock 1

so far so good. Now write a program that uses the same
(already configured) loop device and changes the password
(without changing the underlying file):
loop_open        // lock called, because cipher selected: lock 2
loop_set_status  // unlock called once, lock called twice: lock 3
loop_release     // unlock called: lock 2

repeat this and the lock count will increase towards infinity.

or write a program which changes the transfer function
(without changing the underlying file):
loop_open       // lock called, because cipher selected: lock1 2
loop_set_status // change transfer func:
                // unlock called for transferfunc1 once
                // lock called for transferfunc2 twice
                // lock1 1, lock2 2
loop_release    // lock1 1, lock2 1

Result: completely unused cipher module is locked. 

> It's really only locking once; the issue is that it needs to keep the count
> increased while a loop device references a filter module. The other locking
> between open/release is just temporary.

A scheme in which loop_open and loop_release call lock and unlock
for the transfer function has the problem, that conceptually the
transfer function might change between a loop_open and loop_release call.
(It might even change more than once!).

If you call lock AND unlock twice in "set_status" you can resolve
this problem, but this is simply "double" locking for no clear 
benefit. It only means that the locking count of a transfer function
is calculated this way: 
Number of loop devices which use this function +
Number of open file descriptors of loop devices which use this function.

IMHO a transfer function doesn't belong to the application which opens 
a loop device, but to the loop device itself.

I probably didn't explain it that well in my first posting, but 
after applying the proposed patch, a transfer function stays locked, 
as long as a loop device has a reference to it; the locking count only
is not influenced any longer by "lo_open" and "lo_release".

Locking and unlocking is done via "set_status" and unlocking is also done
via "clr_fd". So the semantic is, that a loop device can only have a transfer
function as long as it is bound. ("set_status" checks this when you try
to configure the loop device for encryption.)

so long
  Ingo

PS: Lets see if I understand the SMP issue correctly:
    Imagine that two processor access two different loop devices
    which use the same transfer module (it has to be two different
    loop devices, because the loop devices are protected against
    multi processor use).
    Then it is possible that two processors call the same
    functions in the transfer module, which in turn access the same
    global module variable, without synchronizing this accesses.
    So we have lots of races right ?

Re: Loop encryption module locking bug (linux-2.4.5).

[Andreas Dilger]

Ingo Rohloff writes:
> PS: Because I try to understand the inner workings of the loop
>     device better, I have a question:
>     In lo_send is a loop: "while (len>0)". How can I configure
>     a loop device, so that this loop is executed more than once.
>     It seems this is only possible if "bh->b_size" is greater
>     than PAGE_CACHE_SIZE. Does this mean, you have to work on
>     a filesystem which uses blocks of a size > PAGE_CACHE_SIZE,
>     or is bh->b_size a fixed value (which is always less than
>     PAGE_CACHE_SIZE) ?

Currently, filesystems must have block size <= PAGE_CACHE_SIZE.
This may not be true in the future, so it is likely that the loop
code is "forward looking" to try to still work if the block size
can exceed the PAGE_CACHE_SIZE.

Cheers, Andreas
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Loop encryption module locking bug (linux-2.4.5).

[Ingo Rohloff]

[-- Attachment #1: Type: text/plain, Size: 2183 bytes --]

Hi,

There is a bug in the locking scheme for the encryption functions,
which can be hooked into the loop device. I have a patch
which resolves the problem. First what happens:

If you do (for example) a losetup -e twofish /dev/loop0 test.lop
the following happens:

The loop0 device gets opened (to use ioctl after that).  Because no cipher
is selected, the loop device hasn't an associated transfer function
(lo->lo_encrypt_type is zero). That means, that lo_open doesn't call a lock
function.

Then you set your password and loop_set_status is called (via ioctl), which
sets lo->lo_encrypt_type to the correct cipher and also calls the
lock function of the cipher. 

When this is done, losetup closes the device and lo_release is called.
Now lo->lo_encrypt_type contains the right value and this leads to
calling the unlock function of the appropriate cipher.

After that the cipher isn't locked any more, so you can 
rmmod the corresponding cipher, regardless of the fact that
the loop device still has hooks into it.

If lo_open doesn't call the cipher lock function and 
lo_release doesn't call the cipher unlock function, the issue
is resolved. (So code gets deleted in the patch.)

xfer_funcs[type]->lock is then called, when a cipher is selected for a
specific loop device. Unlock is called when a cipher is deselected for a
specific loop device. It doesn't matter if the device is opened or
not, which isn't important for the locking anyway.

The patch which is attached, is against linux-2.4.5.

I try to get this patch in the kernel for quite some time, but
it seems I do something wrong (or no one is interested) ? 
Perhaps it will go in this time...

so long
  Ingo

PS: Because I try to understand the inner workings of the loop
    device better, I have a question:
    In lo_send is a loop: "while (len>0)". How can I configure
    a loop device, so that this loop is executed more than once.
    It seems this is only possible if "bh->b_size" is greater
    than PAGE_CACHE_SIZE. Does this mean, you have to work on
    a filesystem which uses blocks of a size > PAGE_CACHE_SIZE,
    or is bh->b_size a fixed value (which is always less than
    PAGE_CACHE_SIZE) ?

[-- Attachment #2: loop-lock.patch --]
[-- Type: text/plain, Size: 979 bytes --]

--- drivers/block/loop.c~	Mon Apr 30 17:59:20 2001
+++ drivers/block/loop.c	Thu May 31 14:41:01 2001
@@ -864,7 +864,7 @@
 static int lo_open(struct inode *inode, struct file *file)
 {
 	struct loop_device *lo;
-	int	dev, type;
+	int	dev;
 
 	if (!inode)
 		return -EINVAL;
@@ -879,10 +879,6 @@
 	lo = &loop_dev[dev];
 	MOD_INC_USE_COUNT;
 	down(&lo->lo_ctl_mutex);
-
-	type = lo->lo_encrypt_type; 
-	if (type && xfer_funcs[type] && xfer_funcs[type]->lock)
-		xfer_funcs[type]->lock(lo);
 	lo->lo_refcnt++;
 	up(&lo->lo_ctl_mutex);
 	return 0;
@@ -891,7 +887,7 @@
 static int lo_release(struct inode *inode, struct file *file)
 {
 	struct loop_device *lo;
-	int	dev, type;
+	int	dev;
 
 	if (!inode)
 		return 0;
@@ -906,11 +902,7 @@
 
 	lo = &loop_dev[dev];
 	down(&lo->lo_ctl_mutex);
-	type = lo->lo_encrypt_type;
 	--lo->lo_refcnt;
-	if (xfer_funcs[type] && xfer_funcs[type]->unlock)
-		xfer_funcs[type]->unlock(lo);
-
 	up(&lo->lo_ctl_mutex);
 	MOD_DEC_USE_COUNT;
 	return 0;