[PATCH]:Return proper error value on failure of dentry_open

[PATCH]:Return proper error value on failure of dentry_open

[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 <vibi_sreenivasan@cms.com>
---
 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

Re: [PATCH]:Return proper error value on failure of dentry_open

[Jiri Slaby]

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.

Re: [PATCH]:Return proper error value on failure of dentry_open

[vibi sreenivasan]

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
> 

[PATCH]:RESEND : Return proper error value on failure of dentry_open

[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 <vibi_sreenivasan@cms.com>
---
 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

Re: [PATCH]:RESEND : Return proper error value on failure of dentry_open

[Jiri Slaby]

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?

Re: [PATCH]:Return proper error value on failure of dentry_open

[Jon Masters]

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.

Re: [PATCH]:RESEND : Return proper error value on failure of dentry_open

[Andrew Morton]

On Mon, 01 Jun 2009 10:39:58 +0200 Jiri Slaby <jirislaby@gmail.com> 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'?

Re: [PATCH]:RESEND : Return proper error value on failure of dentry_open

[vibi sreenivasan]

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'?
> 
> 
>