LSM file locking patch is bogus

PATCH In review Matthew Wilcox 6 participants 6 messages Jul 27, 2002 – Aug 9, 2002

Subsystems (3)

VFS Security (LSM) File locking (flock() and fcntl()/lockf())

lore

Share & tools

lore

Matthew Wilcox

author

2002-07-27 22:31 UTC #0 lore raw

PATCH 1/1 LSM file locking patch is bogus

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

fs/locks.c +6 -6

diff -urNX dontdiff linux-2.5.29/fs/locks.c linux-2.5.29-scsi/fs/locks.c
@@ -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;

include/linux/security.h +1 -2

diff -urNX dontdiff linux-2.5.29/include/linux/security.h linux-2.5.29-scsi/include/linux/security.h
@@ -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);

Signature · 1 line

Revolutions do not require corporate support.

Seth Arnold

2002-07-28 03:02 UTC #1 lore raw

Quoted 2 lines · 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!

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

Signature · 1 line

http://sardonix.org/

Linus Torvalds

2002-07-28 03:13 UTC #2 lore raw

Quoted 6 lines · Seth Arnold

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

H. Peter Anvin

2002-08-09 04:00 UTC #3 lore raw

Followup to:  <[email protected]>
By author:    Linus Torvalds <[email protected]>
In newsgroup: linux.dev.fs.devel

Quoted 9 lines

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

Signature · 3 lines

<[email protected]> at work, <[email protected]> in private!
"Unix gives you enough rope to shoot yourself in the foot."
http://www.zytor.com/~hpa/puzzle.txt	<[email protected]>

Greg KH

2002-07-28 18:47 UTC #4 lore raw

In reply to Matthew Wilcox

Quoted 7 lines · 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.

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

greg k-h

Chris Wright

2002-07-29 23:58 UTC #5 lore raw

In reply to Matthew Wilcox

Quoted 7 lines · * Matthew Wilcox ([email protected]) 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.

security/dummy.c +1 -1

@@ -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;
 }

security/capability.c +1 -1

@@ -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

Signature · 1 line

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net