* [PATCH] ima: Fix NULL pointer dereference in ima_file_hash
@ 2020-09-16 12:05 KP Singh
2020-09-16 12:41 ` Florent Revest
0 siblings, 1 reply; 3+ messages in thread
From: KP Singh @ 2020-09-16 12:05 UTC (permalink / raw)
To: linux-kernel, linux-security-module
Cc: Mimi Zohar, James Morris, Kees Cook, Lakshmi Ramasubramanian,
Jann Horn, Florent Revest
From: KP Singh <kpsingh@google.com>
ima_file_hash can be called when there is no iint->ima_hash available
even though the inode exists in the integrity cache.
An example where this can happen (suggested by Jann Horn):
Process A does:
while(1) {
unlink("/tmp/imafoo");
fd = open("/tmp/imafoo", O_RDWR|O_CREAT|O_TRUNC, 0700);
if (fd == -1) {
perror("open");
continue;
}
write(fd, "A", 1);
close(fd);
}
and Process B does:
while (1) {
int fd = open("/tmp/imafoo", O_RDONLY);
if (fd == -1)
continue;
char *mapping = mmap(NULL, 0x1000, PROT_READ|PROT_EXEC,
MAP_PRIVATE, fd, 0);
if (mapping != MAP_FAILED)
munmap(mapping, 0x1000);
close(fd);
}
Due to the race to get the iint->mutex between ima_file_hash and
process_measurement iint->ima_hash could still be NULL.
Fixes: 6beea7afcc72 ("ima: add the ability to query the cached hash of a given file")
Signed-off-by: KP Singh <kpsingh@google.com>
---
security/integrity/ima/ima_main.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..4c86cd4eece0 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -531,6 +531,16 @@ int ima_file_hash(struct file *file, char *buf, size_t buf_size)
return -EOPNOTSUPP;
mutex_lock(&iint->mutex);
+
+ /*
+ * ima_file_hash can be called when ima_collect_measurement has still
+ * not been called, we might not always have a hash.
+ */
+ if (!iint->ima_hash) {
+ mutex_unlock(&iint->mutex);
+ return -EOPNOTSUPP;
+ }
+
if (buf) {
size_t copied_size;
--
2.28.0.618.gf4bc123cb7-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ima: Fix NULL pointer dereference in ima_file_hash
2020-09-16 12:05 [PATCH] ima: Fix NULL pointer dereference in ima_file_hash KP Singh
@ 2020-09-16 12:41 ` Florent Revest
2020-09-16 12:47 ` KP Singh
0 siblings, 1 reply; 3+ messages in thread
From: Florent Revest @ 2020-09-16 12:41 UTC (permalink / raw)
To: KP Singh, linux-kernel, linux-security-module
Cc: Mimi Zohar, James Morris, Kees Cook, Lakshmi Ramasubramanian, Jann Horn
Reviewed-by: Florent Revest <revest@chromium.org>
On Wed, 2020-09-16 at 12:05 +0000, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> ima_file_hash can be called when there is no iint->ima_hash available
> even though the inode exists in the integrity cache.
>
> An example where this can happen (suggested by Jann Horn):
>
> Process A does:
>
> while(1) {
> unlink("/tmp/imafoo");
> fd = open("/tmp/imafoo", O_RDWR|O_CREAT|O_TRUNC, 0700);
> if (fd == -1) {
> perror("open");
> continue;
> }
> write(fd, "A", 1);
> close(fd);
> }
>
> and Process B does:
>
> while (1) {
> int fd = open("/tmp/imafoo", O_RDONLY);
> if (fd == -1)
> continue;
> char *mapping = mmap(NULL, 0x1000, PROT_READ|PROT_EXEC,
> MAP_PRIVATE, fd, 0);
> if (mapping != MAP_FAILED)
> munmap(mapping, 0x1000);
> close(fd);
> }
>
> Due to the race to get the iint->mutex between ima_file_hash and
> process_measurement iint->ima_hash could still be NULL.
>
> Fixes: 6beea7afcc72 ("ima: add the ability to query the cached hash
> of a given file")
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
> security/integrity/ima/ima_main.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/security/integrity/ima/ima_main.c
> b/security/integrity/ima/ima_main.c
> index 8a91711ca79b..4c86cd4eece0 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -531,6 +531,16 @@ int ima_file_hash(struct file *file, char *buf,
> size_t buf_size)
> return -EOPNOTSUPP;
>
> mutex_lock(&iint->mutex);
> +
> + /*
> + * ima_file_hash can be called when ima_collect_measurement has
> still
> + * not been called, we might not always have a hash.
> + */
> + if (!iint->ima_hash) {
> + mutex_unlock(&iint->mutex);
> + return -EOPNOTSUPP;
> + }
> +
> if (buf) {
> size_t copied_size;
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ima: Fix NULL pointer dereference in ima_file_hash
2020-09-16 12:41 ` Florent Revest
@ 2020-09-16 12:47 ` KP Singh
0 siblings, 0 replies; 3+ messages in thread
From: KP Singh @ 2020-09-16 12:47 UTC (permalink / raw)
To: Florent Revest
Cc: open list, Linux Security Module list, Mimi Zohar, James Morris,
Kees Cook, Lakshmi Ramasubramanian, Jann Horn
Somehow this patch does not seem to have been picked up by any of the
mailing list archives and I am not sure if this was delivered to the
lists. I will send a v2 and add Florent's Reviewed-by tag.
On Wed, Sep 16, 2020 at 2:41 PM Florent Revest <revest@chromium.org> wrote:
>
> Reviewed-by: Florent Revest <revest@chromium.org>
>
> On Wed, 2020-09-16 at 12:05 +0000, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > ima_file_hash can be called when there is no iint->ima_hash available
> > even though the inode exists in the integrity cache.
> >
> > An example where this can happen (suggested by Jann Horn):
> >
> > Process A does:
> >
> > while(1) {
> > unlink("/tmp/imafoo");
> > fd = open("/tmp/imafoo", O_RDWR|O_CREAT|O_TRUNC, 0700);
> > if (fd == -1) {
> > perror("open");
> > continue;
> > }
> > write(fd, "A", 1);
> > close(fd);
> > }
> >
> > and Process B does:
> >
> > while (1) {
> > int fd = open("/tmp/imafoo", O_RDONLY);
> > if (fd == -1)
> > continue;
> > char *mapping = mmap(NULL, 0x1000, PROT_READ|PROT_EXEC,
> > MAP_PRIVATE, fd, 0);
> > if (mapping != MAP_FAILED)
> > munmap(mapping, 0x1000);
> > close(fd);
> > }
> >
> > Due to the race to get the iint->mutex between ima_file_hash and
> > process_measurement iint->ima_hash could still be NULL.
> >
> > Fixes: 6beea7afcc72 ("ima: add the ability to query the cached hash
> > of a given file")
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> > security/integrity/ima/ima_main.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index 8a91711ca79b..4c86cd4eece0 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -531,6 +531,16 @@ int ima_file_hash(struct file *file, char *buf,
> > size_t buf_size)
> > return -EOPNOTSUPP;
> >
> > mutex_lock(&iint->mutex);
> > +
> > + /*
> > + * ima_file_hash can be called when ima_collect_measurement has
> > still
> > + * not been called, we might not always have a hash.
> > + */
> > + if (!iint->ima_hash) {
> > + mutex_unlock(&iint->mutex);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > if (buf) {
> > size_t copied_size;
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-09-16 20:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-16 12:05 [PATCH] ima: Fix NULL pointer dereference in ima_file_hash KP Singh
2020-09-16 12:41 ` Florent Revest
2020-09-16 12:47 ` KP Singh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).