[PATCH] LSM file locking patch is bogus

[PATCH] LSM file locking patch is bogus

[Matthew Wilcox]

 - Remove third argument from file_lock security op.  Whether the lock is
   blocking or not cannot make any difference to a security module!
 - Fix the call in sys_flock to pass the translated lock command, not the
   original.
 - Add a call in fcntl_setlease.  If they're going to know about two types
   of lock, let's tell them about the third too.

diff -urNX dontdiff linux-2.5.29/fs/locks.c linux-2.5.29-scsi/fs/locks.c
--- linux-2.5.29/fs/locks.c	2002-07-27 12:09:31.000000000 -0600
+++ linux-2.5.29-scsi/fs/locks.c	2002-07-27 15:20:41.000000000 -0600
@@ -1242,6 +1242,9 @@
 		return -EACCES;
 	if (!S_ISREG(inode->i_mode))
 		return -EINVAL;
+	error = security_ops->file_lock(filp, arg);
+	if (error)
+		return error;
 
 	lock_kernel();
 
@@ -1359,8 +1362,7 @@
 	if (error < 0)
 		goto out_putf;
 
-	error = security_ops->file_lock(filp, cmd,
-					(cmd & LOCK_NB) ? 0 : 1);
+	error = security_ops->file_lock(filp, lock->fl_type);
 	if (error)
 		goto out_putf;
 
@@ -1494,8 +1496,7 @@
 		goto out;
 	}
 
-	error = security_ops->file_lock(filp, file_lock->fl_type,
-	                                cmd == F_SETLKW);
+	error = security_ops->file_lock(filp, file_lock->fl_type);
 	if (error)
 		goto out;
 
@@ -1618,8 +1619,7 @@
 		goto out;
 	}
 
-	error = security_ops->file_lock(filp, file_lock->fl_type,
-					cmd == F_SETLKW64);
+	error = security_ops->file_lock(filp, file_lock->fl_type);
 	if (error)
 		goto out;
 
diff -urNX dontdiff linux-2.5.29/include/linux/security.h linux-2.5.29-scsi/include/linux/security.h
--- linux-2.5.29/include/linux/security.h	2002-07-27 12:09:31.000000000 -0600
+++ linux-2.5.29-scsi/include/linux/security.h	2002-07-27 15:20:16.000000000 -0600
@@ -407,7 +407,6 @@
  *	@file contains the file structure.
  *	@cmd contains the posix-translated lock operation to perform
  *	(e.g. F_RDLCK, F_WRLCK).
- *	@blocking indicates if the request is for a blocking lock.
  *	Return 0 if permission is granted.
  * @file_fcntl:
  *	Check permission before allowing the file operation specified by @cmd
@@ -753,7 +752,7 @@
 	int (*file_mmap) (struct file * file,
 			  unsigned long prot, unsigned long flags);
 	int (*file_mprotect) (struct vm_area_struct * vma, unsigned long prot);
-	int (*file_lock) (struct file * file, unsigned int cmd, int blocking);
+	int (*file_lock) (struct file * file, unsigned int cmd);
 	int (*file_fcntl) (struct file * file, unsigned int cmd,
 			   unsigned long arg);
 	int (*file_set_fowner) (struct file * file);

-- 
Revolutions do not require corporate support.

Re: [PATCH] LSM file locking patch is bogus

[Seth Arnold]

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

On Sat, Jul 27, 2002 at 11:31:49PM +0100, Matthew Wilcox wrote:
>  - Remove third argument from file_lock security op.  Whether the lock is
>    blocking or not cannot make any difference to a security module!

Matthew, thanks for the patch. However, I'm not sure that the "blocking"
field can be removed without opening up some race conditions.

Please see the following analysis of the situation by Antony Edwards,
and make sure that removing the blocking field is still safe:

http://mail.wirex.com/pipermail/linux-security-module/2002-January/002568.html

(I'd be more than happy to forward the original email to anyone who
dislikes web browsers as much as I do. Please mail me privately if you
would like me to send you a copy.)

-- 
http://sardonix.org/

[-- Attachment #2: Type: application/pgp-signature, Size: 232 bytes --]

Re: [PATCH] LSM file locking patch is bogus

[Linus Torvalds]

On Sat, 27 Jul 2002, Seth Arnold wrote:
>
> Matthew, thanks for the patch. However, I'm not sure that the "blocking"
> field can be removed without opening up some race conditions.
>
> Please see the following analysis of the situation by Antony Edwards,
> and make sure that removing the blocking field is still safe:

This should be a non-issue, since current 2.5.x passes the "struct file"
pointer around, not the file descriptor. So there should be no race,
because everybody uses the same "struct file *" everywhere, and nobody
re-gets the file pointer twice any more.

(Yeah, I'm too lazy to double-check that this is 100% true, but it
_should_ all have been fixed a few weeks ago).

		Linus

Re: [PATCH] LSM file locking patch is bogus

[Greg KH]

On Sat, Jul 27, 2002 at 11:31:49PM +0100, Matthew Wilcox wrote:
> 
>  - Remove third argument from file_lock security op.  Whether the lock is
>    blocking or not cannot make any difference to a security module!
>  - Fix the call in sys_flock to pass the translated lock command, not the
>    original.
>  - Add a call in fcntl_setlease.  If they're going to know about two types
>    of lock, let's tell them about the third too.

Thanks a lot for the patch, looks like Linus already took it.

greg k-h

Re: [PATCH] LSM file locking patch is bogus

[Chris Wright]

* Matthew Wilcox (matthew@wil.cx) wrote:
> 
>  - Remove third argument from file_lock security op.  Whether the lock is
>    blocking or not cannot make any difference to a security module!
>  - Fix the call in sys_flock to pass the translated lock command, not the
>    original.
>  - Add a call in fcntl_setlease.  If they're going to know about two types
>    of lock, let's tell them about the third too.

Matthew, thanks for the patch.  It is incomplete, however, as the code
that uses this hook needs to be updated as well.

Linus, the patch below is relative to Matthew Wilcox's patch, and
adjusts the superuser and capability modules according to the change in
the interface.

--- 2.5.29-flock/security/dummy.c	Mon Jul 22 14:44:49 2002
+++ 2.5.29/security/dummy.c	Mon Jul 29 16:54:52 2002
@@ -366,7 +366,7 @@
 	return 0;
 }
 
-static int dummy_file_lock (struct file *file, unsigned int cmd, int blocking)
+static int dummy_file_lock (struct file *file, unsigned int cmd)
 {
 	return 0;
 }
--- 2.5.29-flock/security/capability.c	Mon Jul 22 14:17:37 2002
+++ 2.5.29/security/capability.c	Mon Jul 29 16:54:28 2002
@@ -464,7 +464,7 @@
 	return 0;
 }
 
-static int cap_file_lock (struct file *file, unsigned int cmd, int blocking)
+static int cap_file_lock (struct file *file, unsigned int cmd)
 {
 	return 0;
 }

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] LSM file locking patch is bogus

[H. Peter Anvin]

Followup to:  <Pine.LNX.4.44.0207272011001.3897-100000@home.transmeta.com>
By author:    Linus Torvalds <torvalds@transmeta.com>
In newsgroup: linux.dev.fs.devel
> 
> This should be a non-issue, since current 2.5.x passes the "struct file"
> pointer around, not the file descriptor. So there should be no race,
> because everybody uses the same "struct file *" everywhere, and nobody
> re-gets the file pointer twice any more.
> 
> (Yeah, I'm too lazy to double-check that this is 100% true, but it
> _should_ all have been fixed a few weeks ago).
> 

Does that mean that opening /proc/*/fd/* actually dup()s the file
descriptor as one should expect?

	-hpa
-- 
<hpa@transmeta.com> at work, <hpa@zytor.com> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt	<amsp@zytor.com>