Reposting unchanged, but with Eric's Ack and suggestion.
This patchset separates the incrementing/decrementing of the i_readcount, in
the VFS layer, from other IMA functionality, by replacing the current
ima_counts_get() call with i_readcount_inc(). Its unclear whether this call to
increment i_readcount should be made earlier, like i_writecount. Currently the
call is situated immediately after the switch from put_filp() to fput() for
cleanup.
The patch ordering is a bit redundant in order to leave removing the ifdef
around i_readcount until the last patch. The first four patches: redefines
i_readcount as atomic, defines i_readcount_inc/dec(), moves the IMA
functionality in ima_counts_get() to ima_file_check(), and removes the IMA
imbalance code, simplifying IMA. The last patch removes the ifdef around
i_readcount, making i_readcount into a "first class inode citizen".
The generic_setlease code could then take advantage of i_readcount to help
eliminate some of the race conditions, but not all. For now, Eric suggested
upstreaming patches 1-4 and leaving the 5th patch until there is a second user.
Then we can either unconditionally compile it in, or we can wrap it in
something that either user can select in the Kconfig. Both Bruce and I agree
with his suggestion.
Changelog:
- renamed iget/iput_readcount to i_readcount_inc/dec (Dave Chinner's suggestion)
- finished cleaning up the locking, so that: (based on Eric's comment.)
- i_lock is not required
- i_mutex is taken when making measurement decisions to prevent
file metadata(eg. permissions, xattr) changing from under it.
- iint->mutex is taken when accessing/modifying the iint structure.
- Based on the previous posting discussion, i_readcount is now defined as
atomic.
Mimi
Mimi Zohar (5):
IMA: convert i_readcount to atomic
IMA: define readcount functions
IMA: maintain i_readcount in the VFS layer
IMA: remove IMA imbalance checking
IMA: making i_readcount a first class inode citizen
fs/file_table.c | 5 +-
fs/open.c | 3 +-
include/linux/fs.h | 16 ++++-
include/linux/ima.h | 6 --
security/integrity/ima/ima_iint.c | 5 --
security/integrity/ima/ima_main.c | 130 ++++---------------------------------
6 files changed, 31 insertions(+), 134 deletions(-)
--
1.7.2.2
Convert the inode's i_readcount from an unsigned int to atomic.
Signed-off-by: Mimi Zohar <[email protected]>
Acked-by: Eric Paris <[email protected]>
---
include/linux/fs.h | 3 +--
security/integrity/ima/ima_iint.c | 7 ++++---
security/integrity/ima/ima_main.c | 11 ++++++-----
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 334d68a..442fceb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -787,8 +787,7 @@ struct inode {
unsigned int i_flags;
#ifdef CONFIG_IMA
- /* protected by i_lock */
- unsigned int i_readcount; /* struct files open RO */
+ atomic_t i_readcount; /* struct files open RO */
#endif
atomic_t i_writecount;
#ifdef CONFIG_SECURITY
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index c442e47..f005355 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,10 +137,11 @@ void ima_inode_free(struct inode *inode)
{
struct ima_iint_cache *iint;
- if (inode->i_readcount)
- printk(KERN_INFO "%s: readcount: %u\n", __func__, inode->i_readcount);
+ if (atomic_read(&inode->i_readcount))
+ printk(KERN_INFO "%s: readcount: %u\n", __func__,
+ atomic_read(&inode->i_readcount));
- inode->i_readcount = 0;
+ atomic_set(&inode->i_readcount, 0);
if (!IS_IMA(inode))
return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 203de97..6e8cb93 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -113,7 +113,7 @@ void ima_counts_get(struct file *file)
goto out;
if (mode & FMODE_WRITE) {
- if (inode->i_readcount && IS_IMA(inode))
+ if (atomic_read(&inode->i_readcount) && IS_IMA(inode))
send_tomtou = true;
goto out;
}
@@ -127,7 +127,7 @@ void ima_counts_get(struct file *file)
out:
/* remember the vfs deals with i_writecount */
if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
- inode->i_readcount++;
+ atomic_inc(&inode->i_readcount);
spin_unlock(&inode->i_lock);
@@ -149,15 +149,16 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
assert_spin_locked(&inode->i_lock);
if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
- if (unlikely(inode->i_readcount == 0)) {
+ if (unlikely(atomic_read(&inode->i_readcount) == 0)) {
if (!ima_limit_imbalance(file)) {
printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
- __func__, inode->i_readcount);
+ __func__,
+ atomic_read(&inode->i_readcount));
dump_stack();
}
return;
}
- inode->i_readcount--;
+ atomic_dec(&inode->i_readcount);
}
}
--
1.7.2.2
Define i_readcount_inc/dec() functions to be called from the VFS layer.
Changelog:
- renamed iget/iput_readcount to i_readcount_inc/dec (Dave Chinner's suggestion)
- removed i_lock in iput_readcount() (based on comments:Dave Chinner,Eric Paris)
Signed-off-by: Mimi Zohar <[email protected]>
Acked-by: Eric Paris <[email protected]>
---
include/linux/fs.h | 20 ++++++++++++++++++++
1 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 442fceb..6b7e2fb 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2176,6 +2176,26 @@ static inline void allow_write_access(struct file *file)
if (file)
atomic_inc(&file->f_path.dentry->d_inode->i_writecount);
}
+#ifdef CONFIG_IMA
+static inline void i_readcount_dec(struct inode *inode)
+{
+ BUG_ON(!atomic_read(&inode->i_readcount));
+ atomic_dec(&inode->i_readcount);
+}
+static inline void i_readcount_inc(struct inode *inode)
+{
+ atomic_inc(&inode->i_readcount);
+}
+#else
+static inline void i_readcount_dec(struct inode *inode)
+{
+ return;
+}
+static inline void i_readcount_inc(struct inode *inode)
+{
+ return;
+}
+#endif
extern int do_pipe_flags(int *, int);
extern struct file *create_read_pipe(struct file *f, int flags);
extern struct file *create_write_pipe(int flags);
--
1.7.2.2
ima_counts_get() updated the readcount and invalidated the PCR,
as necessary. Only update the i_readcount in the VFS layer.
Move the PCR invalidation checks to ima_file_check(), where it
belongs.
Maintaining the i_readcount in the VFS layer, will allow other
subsystems to use i_readcount.
Signed-off-by: Mimi Zohar <[email protected]>
Acked-by: Eric Paris <[email protected]>
---
fs/file_table.c | 5 ++++-
fs/open.c | 3 ++-
include/linux/ima.h | 6 ------
security/integrity/ima/ima_iint.c | 2 --
security/integrity/ima/ima_main.c | 25 ++++++++-----------------
5 files changed, 14 insertions(+), 27 deletions(-)
diff --git a/fs/file_table.c b/fs/file_table.c
index c3dee38..0c724de 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -190,7 +190,8 @@ struct file *alloc_file(struct path *path, fmode_t mode,
file_take_write(file);
WARN_ON(mnt_clone_write(path->mnt));
}
- ima_counts_get(file);
+ if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+ i_readcount_inc(path->dentry->d_inode);
return file;
}
EXPORT_SYMBOL(alloc_file);
@@ -251,6 +252,8 @@ static void __fput(struct file *file)
fops_put(file->f_op);
put_pid(file->f_owner.pid);
file_sb_list_del(file);
+ if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+ i_readcount_dec(inode);
if (file->f_mode & FMODE_WRITE)
drop_file_write_access(file);
file->f_path.dentry = NULL;
diff --git a/fs/open.c b/fs/open.c
index 4197b9e..0d485c5 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -688,7 +688,8 @@ static struct file *__dentry_open(struct dentry *dentry, struct vfsmount *mnt,
if (error)
goto cleanup_all;
}
- ima_counts_get(f);
+ if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
+ i_readcount_inc(inode);
f->f_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 975837e..09e6e62 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,6 @@ extern void ima_inode_free(struct inode *inode);
extern int ima_file_check(struct file *file, int mask);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
-extern void ima_counts_get(struct file *file);
#else
static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -53,10 +52,5 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
return 0;
}
-static inline void ima_counts_get(struct file *file)
-{
- return;
-}
-
#endif /* CONFIG_IMA_H */
#endif /* _LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index f005355..68efe3b 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -141,8 +141,6 @@ void ima_inode_free(struct inode *inode)
printk(KERN_INFO "%s: readcount: %u\n", __func__,
atomic_read(&inode->i_readcount));
- atomic_set(&inode->i_readcount, 0);
-
if (!IS_IMA(inode))
return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6e8cb93..69b4856 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -86,17 +86,16 @@ out:
}
/*
- * ima_counts_get - increment file counts
+ * ima_rdwr_violation_check
*
- * Maintain read/write counters for all files, but only
- * invalidate the PCR for measured files:
+ * Only invalidate the PCR for measured files:
* - Opening a file for write when already open for read,
* results in a time of measure, time of use (ToMToU) error.
* - Opening a file for read when already open for write,
* could result in a file measurement error.
*
*/
-void ima_counts_get(struct file *file)
+static void ima_rdwr_violation_check(struct file *file)
{
struct dentry *dentry = file->f_path.dentry;
struct inode *inode = dentry->d_inode;
@@ -104,13 +103,10 @@ void ima_counts_get(struct file *file)
int rc;
bool send_tomtou = false, send_writers = false;
- if (!S_ISREG(inode->i_mode))
+ if (!S_ISREG(inode->i_mode) || !ima_initialized)
return;
- spin_lock(&inode->i_lock);
-
- if (!ima_initialized)
- goto out;
+ mutex_lock(&inode->i_mutex); /* file metadata: permissions, xattr */
if (mode & FMODE_WRITE) {
if (atomic_read(&inode->i_readcount) && IS_IMA(inode))
@@ -125,11 +121,7 @@ void ima_counts_get(struct file *file)
if (atomic_read(&inode->i_writecount) > 0)
send_writers = true;
out:
- /* remember the vfs deals with i_writecount */
- if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
- atomic_inc(&inode->i_readcount);
-
- spin_unlock(&inode->i_lock);
+ mutex_unlock(&inode->i_mutex);
if (send_tomtou)
ima_add_violation(inode, dentry->d_name.name, "invalid_pcr",
@@ -158,7 +150,6 @@ static void ima_dec_counts(struct inode *inode, struct file *file)
}
return;
}
- atomic_dec(&inode->i_readcount);
}
}
@@ -203,8 +194,7 @@ static void ima_file_free_noiint(struct inode *inode, struct file *file)
* ima_file_free - called on __fput()
* @file: pointer to file structure being freed
*
- * Flag files that changed, based on i_version;
- * and decrement the i_readcount.
+ * Flag files that changed, based on i_version
*/
void ima_file_free(struct file *file)
{
@@ -318,6 +308,7 @@ int ima_file_check(struct file *file, int mask)
{
int rc;
+ ima_rdwr_violation_check(file);
rc = process_measurement(file, file->f_dentry->d_name.name,
mask & (MAY_READ | MAY_WRITE | MAY_EXEC),
FILE_CHECK);
--
1.7.2.2
Now that i_readcount is maintained by the VFS layer, remove the
imbalance checking in IMA. Cleans up the IMA code nicely.
Signed-off-by: Mimi Zohar <[email protected]>
Acked-by: Eric Paris <[email protected]>
---
security/integrity/ima/ima_iint.c | 4 --
security/integrity/ima/ima_main.c | 104 ++-----------------------------------
2 files changed, 4 insertions(+), 104 deletions(-)
diff --git a/security/integrity/ima/ima_iint.c b/security/integrity/ima/ima_iint.c
index 68efe3b..4ae7304 100644
--- a/security/integrity/ima/ima_iint.c
+++ b/security/integrity/ima/ima_iint.c
@@ -137,10 +137,6 @@ void ima_inode_free(struct inode *inode)
{
struct ima_iint_cache *iint;
- if (atomic_read(&inode->i_readcount))
- printk(KERN_INFO "%s: readcount: %u\n", __func__,
- atomic_read(&inode->i_readcount));
-
if (!IS_IMA(inode))
return;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 69b4856..2df9021 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -36,55 +36,6 @@ static int __init hash_setup(char *str)
}
__setup("ima_hash=", hash_setup);
-struct ima_imbalance {
- struct hlist_node node;
- unsigned long fsmagic;
-};
-
-/*
- * ima_limit_imbalance - emit one imbalance message per filesystem type
- *
- * Maintain list of filesystem types that do not measure files properly.
- * Return false if unknown, true if known.
- */
-static bool ima_limit_imbalance(struct file *file)
-{
- static DEFINE_SPINLOCK(ima_imbalance_lock);
- static HLIST_HEAD(ima_imbalance_list);
-
- struct super_block *sb = file->f_dentry->d_sb;
- struct ima_imbalance *entry;
- struct hlist_node *node;
- bool found = false;
-
- rcu_read_lock();
- hlist_for_each_entry_rcu(entry, node, &ima_imbalance_list, node) {
- if (entry->fsmagic == sb->s_magic) {
- found = true;
- break;
- }
- }
- rcu_read_unlock();
- if (found)
- goto out;
-
- entry = kmalloc(sizeof(*entry), GFP_NOFS);
- if (!entry)
- goto out;
- entry->fsmagic = sb->s_magic;
- spin_lock(&ima_imbalance_lock);
- /*
- * we could have raced and something else might have added this fs
- * to the list, but we don't really care
- */
- hlist_add_head_rcu(&entry->node, &ima_imbalance_list);
- spin_unlock(&ima_imbalance_lock);
- printk(KERN_INFO "IMA: unmeasured files on fsmagic: %lX\n",
- entry->fsmagic);
-out:
- return found;
-}
-
/*
* ima_rdwr_violation_check
*
@@ -131,65 +82,20 @@ out:
"open_writers");
}
-/*
- * Decrement ima counts
- */
-static void ima_dec_counts(struct inode *inode, struct file *file)
-{
- mode_t mode = file->f_mode;
-
- assert_spin_locked(&inode->i_lock);
-
- if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
- if (unlikely(atomic_read(&inode->i_readcount) == 0)) {
- if (!ima_limit_imbalance(file)) {
- printk(KERN_INFO "%s: open/free imbalance (r:%u)\n",
- __func__,
- atomic_read(&inode->i_readcount));
- dump_stack();
- }
- return;
- }
- }
-}
-
static void ima_check_last_writer(struct ima_iint_cache *iint,
struct inode *inode,
struct file *file)
{
mode_t mode = file->f_mode;
- BUG_ON(!mutex_is_locked(&iint->mutex));
- assert_spin_locked(&inode->i_lock);
-
+ mutex_lock(&iint->mutex);
if (mode & FMODE_WRITE &&
atomic_read(&inode->i_writecount) == 1 &&
iint->version != inode->i_version)
iint->flags &= ~IMA_MEASURED;
-}
-
-static void ima_file_free_iint(struct ima_iint_cache *iint, struct inode *inode,
- struct file *file)
-{
- mutex_lock(&iint->mutex);
- spin_lock(&inode->i_lock);
-
- ima_dec_counts(inode, file);
- ima_check_last_writer(iint, inode, file);
-
- spin_unlock(&inode->i_lock);
mutex_unlock(&iint->mutex);
}
-static void ima_file_free_noiint(struct inode *inode, struct file *file)
-{
- spin_lock(&inode->i_lock);
-
- ima_dec_counts(inode, file);
-
- spin_unlock(&inode->i_lock);
-}
-
/**
* ima_file_free - called on __fput()
* @file: pointer to file structure being freed
@@ -205,12 +111,10 @@ void ima_file_free(struct file *file)
return;
iint = ima_iint_find(inode);
+ if (!iint)
+ return;
- if (iint)
- ima_file_free_iint(iint, inode, file);
- else
- ima_file_free_noiint(inode, file);
-
+ ima_check_last_writer(iint, inode, file);
}
static int process_measurement(struct file *file, const unsigned char *filename,
--
1.7.2.2
Finally, remove the ifdef's around i_readcount, making it a full
inode citizen so that other subsystems, such as leases, could use it.
Signed-off-by: Mimi Zohar <[email protected]>
---
include/linux/fs.h | 15 ++-------------
1 files changed, 2 insertions(+), 13 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6b7e2fb..1024da1 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -786,9 +786,7 @@ struct inode {
unsigned int i_flags;
-#ifdef CONFIG_IMA
atomic_t i_readcount; /* struct files open RO */
-#endif
atomic_t i_writecount;
#ifdef CONFIG_SECURITY
void *i_security;
@@ -2176,7 +2174,7 @@ static inline void allow_write_access(struct file *file)
if (file)
atomic_inc(&file->f_path.dentry->d_inode->i_writecount);
}
-#ifdef CONFIG_IMA
+
static inline void i_readcount_dec(struct inode *inode)
{
BUG_ON(!atomic_read(&inode->i_readcount));
@@ -2186,16 +2184,7 @@ static inline void i_readcount_inc(struct inode *inode)
{
atomic_inc(&inode->i_readcount);
}
-#else
-static inline void i_readcount_dec(struct inode *inode)
-{
- return;
-}
-static inline void i_readcount_inc(struct inode *inode)
-{
- return;
-}
-#endif
+
extern int do_pipe_flags(int *, int);
extern struct file *create_read_pipe(struct file *f, int flags);
extern struct file *create_write_pipe(int flags);
--
1.7.2.2
On Thu, Nov 18, 2010 at 3:02 PM, Mimi Zohar <[email protected]> wrote:
>
> This patchset separates the incrementing/decrementing of the i_readcount, in
> the VFS layer, from other IMA functionality, by replacing the current
> ima_counts_get() call with i_readcount_inc(). Its unclear whether this call to
> increment i_readcount should be made earlier, like i_writecount. ?Currently the
> call is situated immediately after the switch from put_filp() to fput() for
> cleanup.
Well, it seems nicer than the situation we have now. So I'm certainly
ok with seeing this merged for 2.6.38 (through the security tree?) if
nobody has objections.
It's a bit sad to have another atomic in the open path, but if the
lease people want this and are ok with just the counter (no races?)
then it seems worth it.
Linus
On Thu, Nov 18, 2010 at 03:31:10PM -0800, Linus Torvalds wrote:
> On Thu, Nov 18, 2010 at 3:02 PM, Mimi Zohar <[email protected]> wrote:
> >
> > This patchset separates the incrementing/decrementing of the i_readcount, in
> > the VFS layer, from other IMA functionality, by replacing the current
> > ima_counts_get() call with i_readcount_inc(). Its unclear whether this call to
> > increment i_readcount should be made earlier, like i_writecount. Currently the
> > call is situated immediately after the switch from put_filp() to fput() for
> > cleanup.
>
> Well, it seems nicer than the situation we have now. So I'm certainly
> ok with seeing this merged for 2.6.38 (through the security tree?) if
> nobody has objections.
>
> It's a bit sad to have another atomic in the open path, but if the
> lease people want this and are ok with just the counter (no races?)
> then it seems worth it.
Having thought about it more, I'm no longer convinced it will be useful
for leases.
It seems attractive to replace the current d_count/i_count checks by an
i_readcount check, but:
1) as long as break_lease() is called before i_readcount_inc(),
there's a window between the two where setlease has no way to
tell whether a read open is about to happen;
2) more importantly, it won't help file servers, which need more
than mutual exclusion between opens and leases.
Number 2 in more detail:
Write leases exist to let a file server (nfsd or Samba) tell a client
that it has exclusive access to a file, so that the client can delay
writes, knowing that it will be notified on lease break (and given a
chance to write back dirty data) before someone else can look at the
file.
But say someone modifies a file on a client and then runs "make" on the
server. The "make" needs to know about the modifications. But make only
stat's the file, doesn't open it....
We can break leases on stat, but on its own that's racy--setlease needs
some way to determine whether a lease is in progress. And i_readlease()
doesn't help there, unless we decide we're going to temporarily
increment that around every stat. (But if another atomic in the open
path is bad, another in the stat path sounds worse--and it's probably
not the semantics ima needs anyway.)
--b.
On Fri, Nov 19, 2010 at 12:50:53PM -0500, J. Bruce Fields wrote:
> On Thu, Nov 18, 2010 at 03:31:10PM -0800, Linus Torvalds wrote:
> > On Thu, Nov 18, 2010 at 3:02 PM, Mimi Zohar <[email protected]> wrote:
> > >
> > > This patchset separates the incrementing/decrementing of the i_readcount, in
> > > the VFS layer, from other IMA functionality, by replacing the current
> > > ima_counts_get() call with i_readcount_inc(). Its unclear whether this call to
> > > increment i_readcount should be made earlier, like i_writecount. Currently the
> > > call is situated immediately after the switch from put_filp() to fput() for
> > > cleanup.
> >
> > Well, it seems nicer than the situation we have now. So I'm certainly
> > ok with seeing this merged for 2.6.38 (through the security tree?) if
> > nobody has objections.
> >
> > It's a bit sad to have another atomic in the open path, but if the
> > lease people want this and are ok with just the counter (no races?)
> > then it seems worth it.
>
> Having thought about it more, I'm no longer convinced it will be useful
> for leases.
>
> It seems attractive to replace the current d_count/i_count checks by an
> i_readcount check, but:
>
> 1) as long as break_lease() is called before i_readcount_inc(),
> there's a window between the two where setlease has no way to
> tell whether a read open is about to happen;
>
> 2) more importantly, it won't help file servers, which need more
> than mutual exclusion between opens and leases.
>
> Number 2 in more detail:
>
> Write leases exist to let a file server (nfsd or Samba) tell a client
> that it has exclusive access to a file, so that the client can delay
> writes, knowing that it will be notified on lease break (and given a
> chance to write back dirty data) before someone else can look at the
> file.
>
> But say someone modifies a file on a client and then runs "make" on the
> server. The "make" needs to know about the modifications. But make only
> stat's the file, doesn't open it....
>
> We can break leases on stat, but on its own that's racy--setlease needs
> some way to determine whether a lease is in progress. And i_readlease()
(err, I meant i_readcount).
> doesn't help there, unless we decide we're going to temporarily
> increment that around every stat. (But if another atomic in the open
> path is bad, another in the stat path sounds worse--and it's probably
> not the semantics ima needs anyway.)
Anyway, so I've got nothing against i_readlease, but I don't see how to
use them for the write lease implementation--it looks to me like we're
better off living with d_count/i_count checks. They give false
positives, but I don't think some false positives are really a problem.
--b.
On Fri, 2010-11-19 at 12:50 -0500, J. Bruce Fields wrote:
> On Thu, Nov 18, 2010 at 03:31:10PM -0800, Linus Torvalds wrote:
> > On Thu, Nov 18, 2010 at 3:02 PM, Mimi Zohar <[email protected]> wrote:
> > >
> > > This patchset separates the incrementing/decrementing of the i_readcount, in
> > > the VFS layer, from other IMA functionality, by replacing the current
> > > ima_counts_get() call with i_readcount_inc(). Its unclear whether this call to
> > > increment i_readcount should be made earlier, like i_writecount. Currently the
> > > call is situated immediately after the switch from put_filp() to fput() for
> > > cleanup.
> >
> > Well, it seems nicer than the situation we have now. So I'm certainly
> > ok with seeing this merged for 2.6.38 (through the security tree?) if
> > nobody has objections.
> >
> > It's a bit sad to have another atomic in the open path, but if the
> > lease people want this and are ok with just the counter (no races?)
> > then it seems worth it.
>
> Having thought about it more, I'm no longer convinced it will be useful
> for leases.
>
> It seems attractive to replace the current d_count/i_count checks by an
> i_readcount check, but:
>
> 1) as long as break_lease() is called before i_readcount_inc(),
> there's a window between the two where setlease has no way to
> tell whether a read open is about to happen;
>
> 2) more importantly, it won't help file servers, which need more
> than mutual exclusion between opens and leases.
>
> Number 2 in more detail:
>
> Write leases exist to let a file server (nfsd or Samba) tell a client
> that it has exclusive access to a file, so that the client can delay
> writes, knowing that it will be notified on lease break (and given a
> chance to write back dirty data) before someone else can look at the
> file.
>
> But say someone modifies a file on a client and then runs "make" on the
> server. The "make" needs to know about the modifications. But make only
> stat's the file, doesn't open it....
Hi Bruce,
IMA (and the proposed EVM/IMA-appraisal patches) detects file change
based on i_version. When the file is closed, if the file has changed,
IMA marks the file as needing to be re-measured. Of course this requires
the filesystem to be mounted with iversion. Don't know if this helps.
Mimi
> We can break leases on stat, but on its own that's racy--setlease needs
> some way to determine whether a lease is in progress. And i_readlease()
> doesn't help there, unless we decide we're going to temporarily
> increment that around every stat. (But if another atomic in the open
> path is bad, another in the stat path sounds worse--and it's probably
> not the semantics ima needs anyway.)
>
> --b.
On Sun, Nov 21, 2010 at 5:18 AM, Mimi Zohar <[email protected]> wrote:
>
> IMA (and the proposed EVM/IMA-appraisal patches) detects file change
> based on i_version. When the file is closed, if the file has changed,
> IMA marks the file as needing to be re-measured. Of course this requires
> the filesystem to be mounted with iversion. Don't know if this helps.
If you only do this at close time, I see a _major_ security hole.
The attacker can just write to the file, and keep it open. Ta-daa,
everybody who reads it sees the new contents, but your IMA logic is
oblivious and thinks it doesn't need to be re-measured.
Linus
On Sun, 2010-11-21 at 09:56 -0800, Linus Torvalds wrote:
> On Sun, Nov 21, 2010 at 5:18 AM, Mimi Zohar <[email protected]> wrote:
> >
> > IMA (and the proposed EVM/IMA-appraisal patches) detects file change
> > based on i_version. When the file is closed, if the file has changed,
> > IMA marks the file as needing to be re-measured. Of course this requires
> > the filesystem to be mounted with iversion. Don't know if this helps.
>
> If you only do this at close time, I see a _major_ security hole.
>
> The attacker can just write to the file, and keep it open. Ta-daa,
> everybody who reads it sees the new contents, but your IMA logic is
> oblivious and thinks it doesn't need to be re-measured.
>
> Linus
Not exactly. While the file remains open for write, it doesn't make any
sense to re-measure the file, as there is nothing preventing the file
from continuing to change. Any measurement would thus be meaningless.
Only after the file closes, does it make sense to re-measure. I did not
mean to imply there isn't any indication of the problem in the
measurement list, there obviously is.
Mimi
On Sun, Nov 21, 2010 at 08:18:18AM -0500, Mimi Zohar wrote:
> On Fri, 2010-11-19 at 12:50 -0500, J. Bruce Fields wrote:
> > On Thu, Nov 18, 2010 at 03:31:10PM -0800, Linus Torvalds wrote:
> > > On Thu, Nov 18, 2010 at 3:02 PM, Mimi Zohar <[email protected]> wrote:
> > > >
> > > > This patchset separates the incrementing/decrementing of the i_readcount, in
> > > > the VFS layer, from other IMA functionality, by replacing the current
> > > > ima_counts_get() call with i_readcount_inc(). Its unclear whether this call to
> > > > increment i_readcount should be made earlier, like i_writecount. Currently the
> > > > call is situated immediately after the switch from put_filp() to fput() for
> > > > cleanup.
> > >
> > > Well, it seems nicer than the situation we have now. So I'm certainly
> > > ok with seeing this merged for 2.6.38 (through the security tree?) if
> > > nobody has objections.
> > >
> > > It's a bit sad to have another atomic in the open path, but if the
> > > lease people want this and are ok with just the counter (no races?)
> > > then it seems worth it.
> >
> > Having thought about it more, I'm no longer convinced it will be useful
> > for leases.
> >
> > It seems attractive to replace the current d_count/i_count checks by an
> > i_readcount check, but:
> >
> > 1) as long as break_lease() is called before i_readcount_inc(),
> > there's a window between the two where setlease has no way to
> > tell whether a read open is about to happen;
> >
> > 2) more importantly, it won't help file servers, which need more
> > than mutual exclusion between opens and leases.
> >
> > Number 2 in more detail:
> >
> > Write leases exist to let a file server (nfsd or Samba) tell a client
> > that it has exclusive access to a file, so that the client can delay
> > writes, knowing that it will be notified on lease break (and given a
> > chance to write back dirty data) before someone else can look at the
> > file.
> >
> > But say someone modifies a file on a client and then runs "make" on the
> > server. The "make" needs to know about the modifications. But make only
> > stat's the file, doesn't open it....
>
> Hi Bruce,
>
> IMA (and the proposed EVM/IMA-appraisal patches) detects file change
> based on i_version. When the file is closed, if the file has changed,
> IMA marks the file as needing to be re-measured. Of course this requires
> the filesystem to be mounted with iversion. Don't know if this helps.
In this example, it's not poor time resolution or anything that prevents
make from noticing the modification; it's the simple fact that the copy
of the file on the server hasn't been modified at all; only the remote
client knows about the modification, and the only way to find out about
the modification is to synchronously call back to the client and let it
write out its dirty data before allowing a local user to read its data
or metadata.
--b.
>
> Mimi
>
> > We can break leases on stat, but on its own that's racy--setlease needs
> > some way to determine whether a lease is in progress. And i_readlease()
> > doesn't help there, unless we decide we're going to temporarily
> > increment that around every stat. (But if another atomic in the open
> > path is bad, another in the stat path sounds worse--and it's probably
> > not the semantics ima needs anyway.)
> >
> > --b.
>
>
>
On Sun, 2010-11-21 at 16:33 -0500, Mimi Zohar wrote:
> On Sun, 2010-11-21 at 09:56 -0800, Linus Torvalds wrote:
> > On Sun, Nov 21, 2010 at 5:18 AM, Mimi Zohar <[email protected]> wrote:
> > >
> > > IMA (and the proposed EVM/IMA-appraisal patches) detects file change
> > > based on i_version. When the file is closed, if the file has changed,
> > > IMA marks the file as needing to be re-measured. Of course this requires
> > > the filesystem to be mounted with iversion. Don't know if this helps.
> >
> > If you only do this at close time, I see a _major_ security hole.
> >
> > The attacker can just write to the file, and keep it open. Ta-daa,
> > everybody who reads it sees the new contents, but your IMA logic is
> > oblivious and thinks it doesn't need to be re-measured.
> >
> > Linus
>
> Not exactly. While the file remains open for write, it doesn't make any
> sense to re-measure the file, as there is nothing preventing the file
> from continuing to change. Any measurement would thus be meaningless.
> Only after the file closes, does it make sense to re-measure. I did not
> mean to imply there isn't any indication of the problem in the
> measurement list, there obviously is.
>
> Mimi
>
To elaborate a bit on Mimi's response - in the case of a malicious
program keeping a file open for write to avoid measurement:
1. as she points out, the reason for i_writecount and i_readcount
is to detect this "open_writer" problem and log it in both the
measurement list and in the audit log.
2. the attacker program itself must have been measured before it
was executed.
dave