dentry_open can return error value on error.
Check that value before calling fput & return proper error value
Signed-off-by: vibi sreenivasan <[email protected]>
---
security/integrity/ima/ima_main.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f4e7266..c58158b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -201,10 +201,10 @@ int ima_path_check(struct path *path, int mask)
}
out:
mutex_unlock(&iint->mutex);
- if (file)
+ if (IS_ERR(file))
fput(file);
kref_put(&iint->refcount, iint_free);
- return 0;
+ return rc;
}
static int process_measurement(struct file *file, const unsigned char *filename,
--
1.6.0
On 06/01/2009 08:11 AM, vibi sreenivasan wrote:
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -201,10 +201,10 @@ int ima_path_check(struct path *path, int mask)
> }
> out:
> mutex_unlock(&iint->mutex);
> - if (file)
> + if (IS_ERR(file))
> fput(file);
This makes no sense at all. If it is IS_ERR, i.e. some negative value,
you don't want to pass it to fput. 'if (file)' was perfectly correct.
hi,
thanks for pointing that out.
> > out:
> > mutex_unlock(&iint->mutex);
> > - if (file)
> > + if (IS_ERR(file))
> > fput(file);
extremely sorry it was
if(!IS_ERR(file))
fput(file);
i will send that patch again
>
> This makes no sense at all. If it is IS_ERR, i.e. some negative value,
> you don't want to pass it to fput. 'if (file)' was perfectly correct.
if(file) is true for file != 0 , ie even if file is a -ve error
value.
so while fput dereference file ,this can cause a bug to be
triggered.
I actually had one.
Thanks & regards
vibi sreenivasan
>
dentry_open can return error value on error.
Check that value before calling fput & return proper error value
Signed-off-by: vibi sreenivasan <[email protected]>
---
security/integrity/ima/ima_main.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f4e7266..c58158b 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -201,10 +201,10 @@ int ima_path_check(struct path *path, int mask)
}
out:
mutex_unlock(&iint->mutex);
- if (file)
+ if (!IS_ERR(file))
fput(file);
kref_put(&iint->refcount, iint_free);
- return 0;
+ return rc;
}
static int process_measurement(struct file *file, const unsigned char *filename,
--
1.6.0
On 06/01/2009 10:27 AM, vibi sreenivasan wrote:
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -201,10 +201,10 @@ int ima_path_check(struct path *path, int mask)
> }
> out:
> mutex_unlock(&iint->mutex);
> - if (file)
> + if (!IS_ERR(file))
> fput(file);
No, IS_ERR won't catch NULL and there is 'file = NULL' on the
dentry_open fail path. I still think 'if (file)' is proper condition.
What bug did you hit?
On Mon, 2009-06-01 at 11:41 +0530, vibi sreenivasan wrote:
> dentry_open can return error value on error.
> Check that value before calling fput & return proper error value
I know you're going to redo this patch. Could you please also post with
a better subject next time? :)
Jon.
On Mon, 01 Jun 2009 10:39:58 +0200 Jiri Slaby <[email protected]> wrote:
> On 06/01/2009 10:27 AM, vibi sreenivasan wrote:
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -201,10 +201,10 @@ int ima_path_check(struct path *path, int mask)
> > }
> > out:
> > mutex_unlock(&iint->mutex);
> > - if (file)
> > + if (!IS_ERR(file))
> > fput(file);
>
> No, IS_ERR won't catch NULL and there is 'file = NULL' on the
> dentry_open fail path. I still think 'if (file)' is proper condition.
>
> What bug did you hit?
if (!(iint->flags & IMA_MEASURED)) {
struct dentry *dentry = dget(path->dentry);
struct vfsmount *mnt = mntget(path->mnt);
file = dentry_open(dentry, mnt, O_RDONLY | O_LARGEFILE,
current_cred());
if (IS_ERR(file)) {
pr_info("%s dentry_open failed\n", dentry->d_name.name);
rc = PTR_ERR(file);
file = NULL;
goto out;
}
rc = get_path_measurement(iint, file, dentry->d_name.name);
}
out:
mutex_unlock(&iint->mutex);
if (file)
fput(file);
kref_put(&iint->refcount, iint_free);
return 0;
}
The handling of `file' looks OK to me.
otoh the function just drops the error code on the floor. Shouldn't it
return `rc'?
hi,
Thanks for spending your time on my patch.
> > What bug did you hit?
i was using linus tree & not linux-next.
in that the code fragment was different
it was
file = dentry_open(dentry, mnt, O_RDONLY, current->cred);
rc = get_path_measurement(iint, file, dentry->d_name.name);
}
out:
mutex_unlock(&iint->mutex);
if (file)
fput(file);
kref_put(&iint->refcount, iint_free);
return 0;
So i hit a bug in fput.
My sincere apologies for taking all of yours valuable time.
I will take care that any of my future contributions will be
based on linux-next.
Thanks & Regards
vibi sreenivasan
> if (!(iint->flags & IMA_MEASURED)) {
> struct dentry *dentry = dget(path->dentry);
> struct vfsmount *mnt = mntget(path->mnt);
>
> file = dentry_open(dentry, mnt, O_RDONLY | O_LARGEFILE,
> current_cred());
> if (IS_ERR(file)) {
> pr_info("%s dentry_open failed\n", dentry->d_name.name);
> rc = PTR_ERR(file);
> file = NULL;
> goto out;
> }
> rc = get_path_measurement(iint, file, dentry->d_name.name);
> }
> out:
> mutex_unlock(&iint->mutex);
> if (file)
> fput(file);
> kref_put(&iint->refcount, iint_free);
> return 0;
> }
>
> The handling of `file' looks OK to me.
>
> otoh the function just drops the error code on the floor. Shouldn't it
> return `rc'?
>
>
>