[PATCH] ecryptfs: Fix explicit null dereference

[PATCH] ecryptfs: Fix explicit null dereference

[Geyslan G. Bem]

If the condition 'ecryptfs_file_to_private(file)' takes false branch
lower_file is dereferenced when NULL.

Caught by Coverity: CIDs 1128834 and 1128833.

Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
 fs/ecryptfs/file.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 2229a74..1c0403a 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct file *lower_file = NULL;
 	long rc = -ENOTTY;
 
-	if (ecryptfs_file_to_private(file))
-		lower_file = ecryptfs_file_to_lower(file);
+	if (!ecryptfs_file_to_private(file))
+		goto out;
+	lower_file = ecryptfs_file_to_lower(file);
 	if (lower_file->f_op->unlocked_ioctl)
 		rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
+out:
 	return rc;
 }
 
@@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct file *lower_file = NULL;
 	long rc = -ENOIOCTLCMD;
 
-	if (ecryptfs_file_to_private(file))
-		lower_file = ecryptfs_file_to_lower(file);
+	if (!ecryptfs_file_to_private(file))
+		goto out;
+	lower_file = ecryptfs_file_to_lower(file);
 	if (lower_file->f_op && lower_file->f_op->compat_ioctl)
 		rc = lower_file->f_op->compat_ioctl(lower_file, cmd, arg);
+out:
 	return rc;
 }
 #endif
-- 
1.8.4.2

[PATCH] ecryptfs: Fix explicit null dereference

[Geyslan G. Bem]

If the condition 'ecryptfs_file_to_private(file)' takes false branch
lower_file is dereferenced when NULL.

Caught by Coverity: CIDs 1128834 and 1128833.

Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
---
 fs/ecryptfs/file.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 2229a74..1c0403a 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct file *lower_file = NULL;
 	long rc = -ENOTTY;
 
-	if (ecryptfs_file_to_private(file))
-		lower_file = ecryptfs_file_to_lower(file);
+	if (!ecryptfs_file_to_private(file))
+		goto out;
+	lower_file = ecryptfs_file_to_lower(file);
 	if (lower_file->f_op->unlocked_ioctl)
 		rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
+out:
 	return rc;
 }
 
@@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	struct file *lower_file = NULL;
 	long rc = -ENOIOCTLCMD;
 
-	if (ecryptfs_file_to_private(file))
-		lower_file = ecryptfs_file_to_lower(file);
+	if (!ecryptfs_file_to_private(file))
+		goto out;
+	lower_file = ecryptfs_file_to_lower(file);
 	if (lower_file->f_op && lower_file->f_op->compat_ioctl)
 		rc = lower_file->f_op->compat_ioctl(lower_file, cmd, arg);
+out:
 	return rc;
 }
 #endif
-- 
1.8.4.2

Re: [PATCH] ecryptfs: Fix explicit null dereference

[Pekka Enberg]

On Thu, Nov 14, 2013 at 8:42 PM, Geyslan G. Bem <geyslan@gmail.com> wrote:
> If the condition 'ecryptfs_file_to_private(file)' takes false branch
> lower_file is dereferenced when NULL.
>
> Caught by Coverity: CIDs 1128834 and 1128833.
>
> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>

Reviewed-by: Pekka Enberg <penberg@kernel.org>

Re: [PATCH] ecryptfs: Fix explicit null dereference

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 2177 bytes --]

On 2013-11-14 15:42:14, Geyslan G. Bem wrote:
> If the condition 'ecryptfs_file_to_private(file)' takes false branch
> lower_file is dereferenced when NULL.
> 
> Caught by Coverity: CIDs 1128834 and 1128833.
> 
> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> ---

Hello - Smatch picked up on this earlier in week and Dan analyzed the
situation here:

  http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/441

I agree with his assessment and proposed the following patch:

  http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/442

It makes Smatch happy and it should also make Coverity happy.

Tyler

>  fs/ecryptfs/file.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
> index 2229a74..1c0403a 100644
> --- a/fs/ecryptfs/file.c
> +++ b/fs/ecryptfs/file.c
> @@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	struct file *lower_file = NULL;
>  	long rc = -ENOTTY;
>  
> -	if (ecryptfs_file_to_private(file))
> -		lower_file = ecryptfs_file_to_lower(file);
> +	if (!ecryptfs_file_to_private(file))
> +		goto out;
> +	lower_file = ecryptfs_file_to_lower(file);
>  	if (lower_file->f_op->unlocked_ioctl)
>  		rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
> +out:
>  	return rc;
>  }
>  
> @@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>  	struct file *lower_file = NULL;
>  	long rc = -ENOIOCTLCMD;
>  
> -	if (ecryptfs_file_to_private(file))
> -		lower_file = ecryptfs_file_to_lower(file);
> +	if (!ecryptfs_file_to_private(file))
> +		goto out;
> +	lower_file = ecryptfs_file_to_lower(file);
>  	if (lower_file->f_op && lower_file->f_op->compat_ioctl)
>  		rc = lower_file->f_op->compat_ioctl(lower_file, cmd, arg);
> +out:
>  	return rc;
>  }
>  #endif
> -- 
> 1.8.4.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ecryptfs: Fix explicit null dereference

[Geyslan Gregório Bem]

2013/11/14 Tyler Hicks <tyhicks@canonical.com>:
> On 2013-11-14 15:42:14, Geyslan G. Bem wrote:
>> If the condition 'ecryptfs_file_to_private(file)' takes false branch
>> lower_file is dereferenced when NULL.
>>
>> Caught by Coverity: CIDs 1128834 and 1128833.
>>
>> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
>> ---
>
> Hello - Smatch picked up on this earlier in week and Dan analyzed the
> situation here:
>
>   http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/441
>
> I agree with his assessment and proposed the following patch:
>
>   http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/442
>
> It makes Smatch happy and it should also make Coverity happy.
>
> Tyler

True. Disregard mine.

Thanks Tyler.

>
>>  fs/ecryptfs/file.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
>> index 2229a74..1c0403a 100644
>> --- a/fs/ecryptfs/file.c
>> +++ b/fs/ecryptfs/file.c
>> @@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>       struct file *lower_file = NULL;
>>       long rc = -ENOTTY;
>>
>> -     if (ecryptfs_file_to_private(file))
>> -             lower_file = ecryptfs_file_to_lower(file);
>> +     if (!ecryptfs_file_to_private(file))
>> +             goto out;
>> +     lower_file = ecryptfs_file_to_lower(file);
>>       if (lower_file->f_op->unlocked_ioctl)
>>               rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
>> +out:
>>       return rc;
>>  }
>>
>> @@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>>       struct file *lower_file = NULL;
>>       long rc = -ENOIOCTLCMD;
>>
>> -     if (ecryptfs_file_to_private(file))
>> -             lower_file = ecryptfs_file_to_lower(file);
>> +     if (!ecryptfs_file_to_private(file))
>> +             goto out;
>> +     lower_file = ecryptfs_file_to_lower(file);
>>       if (lower_file->f_op && lower_file->f_op->compat_ioctl)
>>               rc = lower_file->f_op->compat_ioctl(lower_file, cmd, arg);
>> +out:
>>       return rc;
>>  }
>>  #endif
>> --
>> 1.8.4.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Regards,

Geyslan G. Bem
hackingbits.com

Re: [PATCH] ecryptfs: Fix explicit null dereference

[Tyler Hicks]

[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]

On 2013-11-14 17:58:40, Geyslan Gregório Bem wrote:
> 2013/11/14 Tyler Hicks <tyhicks@canonical.com>:
> > On 2013-11-14 15:42:14, Geyslan G. Bem wrote:
> >> If the condition 'ecryptfs_file_to_private(file)' takes false branch
> >> lower_file is dereferenced when NULL.
> >>
> >> Caught by Coverity: CIDs 1128834 and 1128833.
> >>
> >> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
> >> ---
> >
> > Hello - Smatch picked up on this earlier in week and Dan analyzed the
> > situation here:
> >
> >   http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/441
> >
> > I agree with his assessment and proposed the following patch:
> >
> >   http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/442
> >
> > It makes Smatch happy and it should also make Coverity happy.
> >
> > Tyler
> 
> True. Disregard mine.
> 
> Thanks Tyler.

Thank you! Can I add your Reviewed-by: tag to that patch prior to
pushing it to my next branch?

Tyler

> 
> >
> >>  fs/ecryptfs/file.c | 12 ++++++++----
> >>  1 file changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
> >> index 2229a74..1c0403a 100644
> >> --- a/fs/ecryptfs/file.c
> >> +++ b/fs/ecryptfs/file.c
> >> @@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >>       struct file *lower_file = NULL;
> >>       long rc = -ENOTTY;
> >>
> >> -     if (ecryptfs_file_to_private(file))
> >> -             lower_file = ecryptfs_file_to_lower(file);
> >> +     if (!ecryptfs_file_to_private(file))
> >> +             goto out;
> >> +     lower_file = ecryptfs_file_to_lower(file);
> >>       if (lower_file->f_op->unlocked_ioctl)
> >>               rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
> >> +out:
> >>       return rc;
> >>  }
> >>
> >> @@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >>       struct file *lower_file = NULL;
> >>       long rc = -ENOIOCTLCMD;
> >>
> >> -     if (ecryptfs_file_to_private(file))
> >> -             lower_file = ecryptfs_file_to_lower(file);
> >> +     if (!ecryptfs_file_to_private(file))
> >> +             goto out;
> >> +     lower_file = ecryptfs_file_to_lower(file);
> >>       if (lower_file->f_op && lower_file->f_op->compat_ioctl)
> >>               rc = lower_file->f_op->compat_ioctl(lower_file, cmd, arg);
> >> +out:
> >>       return rc;
> >>  }
> >>  #endif
> >> --
> >> 1.8.4.2
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Regards,
> 
> Geyslan G. Bem
> hackingbits.com
> --
> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] ecryptfs: Fix explicit null dereference

[Geyslan Gregório Bem]

2013/11/14 Tyler Hicks <tyhicks@canonical.com>:
> On 2013-11-14 17:58:40, Geyslan Gregório Bem wrote:
>> 2013/11/14 Tyler Hicks <tyhicks@canonical.com>:
>> > On 2013-11-14 15:42:14, Geyslan G. Bem wrote:
>> >> If the condition 'ecryptfs_file_to_private(file)' takes false branch
>> >> lower_file is dereferenced when NULL.
>> >>
>> >> Caught by Coverity: CIDs 1128834 and 1128833.
>> >>
>> >> Signed-off-by: Geyslan G. Bem <geyslan@gmail.com>
>> >> ---
>> >
>> > Hello - Smatch picked up on this earlier in week and Dan analyzed the
>> > situation here:
>> >
>> >   http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/441
>> >
>> > I agree with his assessment and proposed the following patch:
>> >
>> >   http://article.gmane.org/gmane.comp.file-systems.ecryptfs.general/442
>> >
>> > It makes Smatch happy and it should also make Coverity happy.
>> >
>> > Tyler
>>
>> True. Disregard mine.
>>
>> Thanks Tyler.
>
> Thank you! Can I add your Reviewed-by: tag to that patch prior to
> pushing it to my next branch?
>
> Tyler
>

Sure, Tyler. You can! :)

Thanks again.

>>
>> >
>> >>  fs/ecryptfs/file.c | 12 ++++++++----
>> >>  1 file changed, 8 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
>> >> index 2229a74..1c0403a 100644
>> >> --- a/fs/ecryptfs/file.c
>> >> +++ b/fs/ecryptfs/file.c
>> >> @@ -316,10 +316,12 @@ ecryptfs_unlocked_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> >>       struct file *lower_file = NULL;
>> >>       long rc = -ENOTTY;
>> >>
>> >> -     if (ecryptfs_file_to_private(file))
>> >> -             lower_file = ecryptfs_file_to_lower(file);
>> >> +     if (!ecryptfs_file_to_private(file))
>> >> +             goto out;
>> >> +     lower_file = ecryptfs_file_to_lower(file);
>> >>       if (lower_file->f_op->unlocked_ioctl)
>> >>               rc = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
>> >> +out:
>> >>       return rc;
>> >>  }
>> >>
>> >> @@ -330,10 +332,12 @@ ecryptfs_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>> >>       struct file *lower_file = NULL;
>> >>       long rc = -ENOIOCTLCMD;
>> >>
>> >> -     if (ecryptfs_file_to_private(file))
>> >> -             lower_file = ecryptfs_file_to_lower(file);
>> >> +     if (!ecryptfs_file_to_private(file))
>> >> +             goto out;
>> >> +     lower_file = ecryptfs_file_to_lower(file);
>> >>       if (lower_file->f_op && lower_file->f_op->compat_ioctl)
>> >>               rc = lower_file->f_op->compat_ioctl(lower_file, cmd, arg);
>> >> +out:
>> >>       return rc;
>> >>  }
>> >>  #endif
>> >> --
>> >> 1.8.4.2
>> >>
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
>> >> the body of a message to majordomo@vger.kernel.org
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>> --
>> Regards,
>>
>> Geyslan G. Bem
>> hackingbits.com
>> --
>> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Regards,

Geyslan G. Bem
hackingbits.com