This patch fixes an imbalance message as reported by J.R. Okajima.
The IMA file counters are incremented in ima_path_check. If the
actual open fails, such as ETXTBSY, decrement the counters to
prevent unnecessary imbalance messages.
Signed-off-by: Mimi Zohar <[email protected]>
---
fs/namei.c | 7 +++++++
include/linux/ima.h | 6 ++++++
security/integrity/ima/ima_main.c | 29 ++++++++++++++++++++++++++++-
3 files changed, 41 insertions(+), 1 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 527119a..5abba79 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1758,6 +1758,10 @@ do_last:
goto exit;
}
filp = nameidata_to_filp(&nd, open_flag);
+ if (IS_ERR(filp))
+ ima_counts_put(&nd.path,
+ acc_mode & (MAY_READ | MAY_WRITE |
+ MAY_EXEC));
mnt_drop_write(nd.path.mnt);
return filp;
}
@@ -1812,6 +1816,9 @@ ok:
goto exit;
}
filp = nameidata_to_filp(&nd, open_flag);
+ if (IS_ERR(filp))
+ ima_counts_put(&nd.path,
+ acc_mode & (MAY_READ | MAY_WRITE | MAY_EXEC));
/*
* It is now safe to drop the mnt write
* because the filp has had a write taken
diff --git a/include/linux/ima.h b/include/linux/ima.h
index b1b827d..57ab5fd 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -24,6 +24,7 @@ extern int ima_path_check(struct path *path, int mask, int update_counts);
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);
+extern void ima_counts_put(struct path *path, int mask);
#else
static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -60,5 +61,10 @@ static inline void ima_counts_get(struct file *file)
{
return;
}
+
+static inline void ima_counts_put(struct path *path, int mask);)
+{
+ return;
+}
#endif /* CONFIG_IMA_H */
#endif /* _LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6f61187..101c512 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -238,7 +238,34 @@ out:
}
/*
- * ima_opens_get - increment file counts
+ * ima_counts_put - decrement file counts
+ *
+ * File counts are incremented in ima_path_check. On file open
+ * error, such as ETXTBSY, decrement the counts to prevent
+ * unnecessary imbalance messages.
+ */
+void ima_counts_put(struct path *path, int mask)
+{
+ struct inode *inode = path->dentry->d_inode;
+ struct ima_iint_cache *iint;
+
+ if (!ima_initialized || !S_ISREG(inode->i_mode))
+ return;
+ iint = ima_iint_find_insert_get(inode);
+ if (!iint)
+ return;
+
+ mutex_lock(&iint->mutex);
+ iint->opencount--;
+ if ((mask & MAY_WRITE) || (mask == 0))
+ iint->writecount--;
+ else if (mask & (MAY_READ | MAY_EXEC))
+ iint->readcount--;
+ mutex_unlock(&iint->mutex);
+}
+
+/*
+ * ima_counts_get - increment file counts
*
* - for IPC shm and shmat file.
* - for nfsd exported files.
--
1.6.0.6
This patch fixes an imbalance message as reported by J.R. Okajima.
The IMA file counters are incremented in ima_path_check. If the
actual open fails, such as ETXTBSY, decrement the counters to
prevent unnecessary imbalance messages.
Signed-off-by: Mimi Zohar <[email protected]>
---
fs/namei.c | 7 +++++++
include/linux/ima.h | 6 ++++++
security/integrity/ima/ima_main.c | 29 ++++++++++++++++++++++++++++-
3 files changed, 41 insertions(+), 1 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 527119a..5abba79 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1758,6 +1758,10 @@ do_last:
goto exit;
}
filp = nameidata_to_filp(&nd, open_flag);
+ if (IS_ERR(filp))
+ ima_counts_put(&nd.path,
+ acc_mode & (MAY_READ | MAY_WRITE |
+ MAY_EXEC));
mnt_drop_write(nd.path.mnt);
return filp;
}
@@ -1812,6 +1816,9 @@ ok:
goto exit;
}
filp = nameidata_to_filp(&nd, open_flag);
+ if (IS_ERR(filp))
+ ima_counts_put(&nd.path,
+ acc_mode & (MAY_READ | MAY_WRITE | MAY_EXEC));
/*
* It is now safe to drop the mnt write
* because the filp has had a write taken
diff --git a/include/linux/ima.h b/include/linux/ima.h
index b1b827d..0e3f2a4 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -24,6 +24,7 @@ extern int ima_path_check(struct path *path, int mask, int update_counts);
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);
+extern void ima_counts_put(struct path *path, int mask);
#else
static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -60,5 +61,10 @@ static inline void ima_counts_get(struct file *file)
{
return;
}
+
+static inline void ima_counts_put(struct path *path, int mask)
+{
+ return;
+}
#endif /* CONFIG_IMA_H */
#endif /* _LINUX_IMA_H */
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 6f61187..101c512 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -238,7 +238,34 @@ out:
}
/*
- * ima_opens_get - increment file counts
+ * ima_counts_put - decrement file counts
+ *
+ * File counts are incremented in ima_path_check. On file open
+ * error, such as ETXTBSY, decrement the counts to prevent
+ * unnecessary imbalance messages.
+ */
+void ima_counts_put(struct path *path, int mask)
+{
+ struct inode *inode = path->dentry->d_inode;
+ struct ima_iint_cache *iint;
+
+ if (!ima_initialized || !S_ISREG(inode->i_mode))
+ return;
+ iint = ima_iint_find_insert_get(inode);
+ if (!iint)
+ return;
+
+ mutex_lock(&iint->mutex);
+ iint->opencount--;
+ if ((mask & MAY_WRITE) || (mask == 0))
+ iint->writecount--;
+ else if (mask & (MAY_READ | MAY_EXEC))
+ iint->readcount--;
+ mutex_unlock(&iint->mutex);
+}
+
+/*
+ * ima_counts_get - increment file counts
*
* - for IPC shm and shmat file.
* - for nfsd exported files.
--
1.6.0.6
On Fri, 26 Jun 2009, Mimi Zohar wrote:
> This patch fixes an imbalance message as reported by J.R. Okajima.
> The IMA file counters are incremented in ima_path_check. If the
> actual open fails, such as ETXTBSY, decrement the counters to
> prevent unnecessary imbalance messages.
>
> Signed-off-by: Mimi Zohar <[email protected]>
Don't forget to use "Reported-by:" lines for the bug reporter.
Have you tested this using the original test case?
--
James Morris
<[email protected]>
On Mon, 2009-06-29 at 08:50 +1000, James Morris wrote:
> On Fri, 26 Jun 2009, Mimi Zohar wrote:
>
> > This patch fixes an imbalance message as reported by J.R. Okajima.
> > The IMA file counters are incremented in ima_path_check. If the
> > actual open fails, such as ETXTBSY, decrement the counters to
> > prevent unnecessary imbalance messages.
> >
> > Signed-off-by: Mimi Zohar <[email protected]>
>
> Don't forget to use "Reported-by:" lines for the bug reporter.
Ok
> Have you tested this using the original test case?
Yes, the patch resolves the iint_free() message.
Mimi
Mimi Zohar:
> This patch fixes an imbalance message as reported by J.R. Okajima.
> The IMA file counters are incremented in ima_path_check. If the
> actual open fails, such as ETXTBSY, decrement the counters to
> prevent unnecessary imbalance messages.
Although I have no objection for this fix, I'd like to suggest you to
stop incrementing the counters in ima_path_check().
A while ago IMA_COUNT_LEAVE and ima_counts_get() were introduced, and
now ima_counts_put() appears.
Isn't it easier something like this,
- stop incrementing in ima_path_check().
- call ima_counts_get() in dentry_open() (or similar).
- delete IMA_COUNT_LEAVE/UPDATE and ima_counts_put().
How do you think?
J. R. Okajima
On Mon, 2009-06-29 at 23:12 +0900, [email protected] wrote:
> Mimi Zohar:
> > This patch fixes an imbalance message as reported by J.R. Okajima.
> > The IMA file counters are incremented in ima_path_check. If the
> > actual open fails, such as ETXTBSY, decrement the counters to
> > prevent unnecessary imbalance messages.
>
> Although I have no objection for this fix, I'd like to suggest you to
> stop incrementing the counters in ima_path_check().
> A while ago IMA_COUNT_LEAVE and ima_counts_get() were introduced, and
> now ima_counts_put() appears.
> Isn't it easier something like this,
> - stop incrementing in ima_path_check().
> - call ima_counts_get() in dentry_open() (or similar).
> - delete IMA_COUNT_LEAVE/UPDATE and ima_counts_put().
>
> How do you think?
>
>
> J. R. Okajima
This suggestion has been mentioned before; and yes would definitely
resolve the annoying imbalance and iint_free() messages. But
incrementing/decrementing the pointers automatically each time a file is
opened/closed would defeat their purpose - alerting us that a file was
possibly not measured before being read/executed.
Mimi
Mimi Zohar:
> This suggestion has been mentioned before; and yes would definitely
> resolve the annoying imbalance and iint_free() messages. But
> incrementing/decrementing the pointers automatically each time a file is
> opened/closed would defeat their purpose - alerting us that a file was
> possibly not measured before being read/executed.
I may be wrong since I don't fully understand IMA's purpose, but why did
you create ima_counts_get() and make it call after dentry_open() in
nfsd_open()? Isn't it same thing essentially?
J. R. Okajima
On Tue, 2009-06-30 at 05:36 +0900, [email protected] wrote:
> Mimi Zohar:
> > This suggestion has been mentioned before; and yes would definitely
> > resolve the annoying imbalance and iint_free() messages. But
> > incrementing/decrementing the pointers automatically each time a file is
> > opened/closed would defeat their purpose - alerting us that a file was
> > possibly not measured before being read/executed.
>
> I may be wrong since I don't fully understand IMA's purpose, but why did
> you create ima_counts_get() and make it call after dentry_open() in
> nfsd_open()? Isn't it same thing essentially?
>
>
> J. R. Okajima
NFSv3 is an interesting example. Permission checking is done once,
followed by multiple open/read/close calls. Incrementing the counters in
nfsd_permission() once and decrementing the counters in close, multiple
times, resulted in imbalance messages. True, the solution in this case
was to increment in open and decrement in close, but that was only part
of the solution. The other part of the solution, the important part,
was to add a call to ima_path_check() to measure the file.
The imbalance message did what it was suppose to do - highlight the fact
that a file was read/executed without first being measured.
Mimi
Mimi Zohar:
> NFSv3 is an interesting example. Permission checking is done once,
> followed by multiple open/read/close calls. Incrementing the counters in
> nfsd_permission() once and decrementing the counters in close, multiple
> times, resulted in imbalance messages. True, the solution in this case
> was to increment in open and decrement in close, but that was only part
> of the solution. The other part of the solution, the important part,
> was to add a call to ima_path_check() to measure the file.
Let me make sure.
Does "that was only part of the solution" mean IMA does not work for
NFSD fully? To make IMA work fully, is incrementing before open
absolutely necessary?
J. R. Okajima
On Fri, 2009-07-03 at 13:02 +0900, [email protected] wrote:
> Mimi Zohar:
> > NFSv3 is an interesting example. Permission checking is done once,
> > followed by multiple open/read/close calls. Incrementing the counters in
> > nfsd_permission() once and decrementing the counters in close, multiple
> > times, resulted in imbalance messages. True, the solution in this case
> > was to increment in open and decrement in close, but that was only part
> > of the solution. The other part of the solution, the important part,
> > was to add a call to ima_path_check() to measure the file.
>
> Let me make sure.
> Does "that was only part of the solution" mean IMA does not work for
> NFSD fully? To make IMA work fully, is incrementing before open
> absolutely necessary?
>
> J. R. Okajima
The patch is fine. It adds a call to ima_path_check() in
nfsd_permission(), but delays incrementing the counters to nfsd_open()
and decrementing the counters to nfsd_close() in order for the counters
to be balanced.
Mimi
Mimi Zohar:
> This patch fixes an imbalance message as reported by J.R. Okajima.
> The IMA file counters are incremented in ima_path_check. If the
> actual open fails, such as ETXTBSY, decrement the counters to
> prevent unnecessary imbalance messages.
Unfortunately IMA seems to be still buggy.
may_open() calls ima_path_check() with IMA_COUNT_UPDATE, but may_open()
may fail later with several reasons such like, open-flag mismatch with
inode-flag, capability, the file was executing and get_write_access()
failed, etc.
In these cases, IMA has to maintain its counters too by calling
ima_counts_put() or something, does it?
J. R. Okajima