[PATCH] add lock hook to prevent race

aedwardat_private

Hi,

This is a patch for consideration. It addresses a race condition detected
using the validation tools we (IBM) are working on, and also an
inconsistency in the use of the file_ops->lock hook. It is against the
2.5.2 patch. Sorry for the length of the description -- but that's race
conditions for you.

The race condition is in fcntl(F_SETLK), and occurs because the
file_struct pointer is retrieved from the file descriptor twice. It allows
a user to have the file_ops->fcntl() authorisation performed on a
different file to the one that is eventually locked. The sequence is
as follows:

THREAD-A:
(1) fd1 = open( "myfile", O_RDWR );
(2) fd2 = open( "target_file, O_RDONLY );
(3) fcntl( fd1, F_SETLK, F_WRLOCK );

      KERNEL-A (do_fcntl):
            (4) filp = fget (fd1);
            (5) security_ops->file_ops->fcntl (fd1);
            (6) fcntl_setlk (fd1,cmd)

THREAD-B:
(7) dup2( fd2, fd1 ); /* this closes fd1, dups fd2, and assigns it to fd1
*/

      KERNEL-A (fcntl_setlk):
            (8) filp = fget (fd1) /* this filp is for the target file due
to (7) */
            (9) lock file

Note-1: although above it is written as a whole system call, there is
actually only one line of C (an assignment) in step (7) that needs to
come between (6) and (8). Therefore it can be reproduced without
requiring (e.g.) a bunch of IO interrupts on CPU-A.

Note-2: basic Linux permissions are not vulnerable since their validations
are done in fcntl_setlk after the second lookup. LSM is vulnerable
because the only authorisation that protects the operation is performed
before the second lookup.

As an example of how dangerous this can be, login and su (PAM'd
versions) both try to lock the file /var/run/utmp (world readable). insmod
locks any modules it loads.

The patch puts a file_ops->lock() check in after the second fget (in
fcntl_setlk and fcnlt_setlk64). This has the additional advantage that it
makes lock authorisations consistent. Currently locks set via flock() are
checked by file_ops->lock() but locks set by fcntl(F_SETLK) are not.

Since flock() and fcntl(SETLK) use different constants for the lock types I
move the hook in flock() below the line that translates flock() constants
into fcntl() constants. The LSM hook is now passed fcntl (i.e. POSIX)
style constants rather than the LOCK_SH/LOCK_EX  type. Unfortunately
this means that an extra parameter is required to specify whether the call
is blocking. This solution results in the least change to the base kernel.

Comments?

Cheers,
Antony

(See attached file: lsm-2.5.2-lock.patch)