Re: Fs: Btrfs - Fix possible ERR_PTR() dereferencing.

[Jeff Mahoney <jeffm@suse.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2414 bytes --]

On 9/20/16 2:48 AM, Shailendra Verma wrote:
> This is of course wrong to call kfree() if memdup_user() fails,
> no memory was allocated and the error in the error-valued pointer
> should be returned.
> 
> Reviewed-by: Ravikant Sharma <ravikant.s2@samsung.com>
> Signed-off-by: Shailendra Verma <shailendra.v@samsung.com>

Hi Shailendra -

In all three cases, the return value is set using the error-valued
pointer and the pointer is set to NULL.  kfree() checks to see if the
pointer is NULL and, if so, does nothing.  This allows us to use a
common exit path which is an extremely common pattern in the kernel.  So
there's never any possible ERR_PTR dereferencing happening.

However, in all three cases, the allocation you're checking is the first
in each routine and there's no additional cleanup to do.  So your patch
is an improvement, but it's an improvement in code readability instead
of a bug fix.  I'd ask that you re-submit with a commit message that
reflects that.

Thanks,

-Jeff

> ---
>  fs/btrfs/ioctl.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index da94138..58a40f8 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4512,11 +4512,8 @@ static long btrfs_ioctl_logical_to_ino(struct btrfs_root *root,
>  		return -EPERM;
>  
>  	loi = memdup_user(arg, sizeof(*loi));
> -	if (IS_ERR(loi)) {
> -		ret = PTR_ERR(loi);
> -		loi = NULL;
> -		goto out;
> -	}
> +	if (IS_ERR(loi))
> +		return PTR_ERR(loi);
>  
>  	path = btrfs_alloc_path();
>  	if (!path) {


> @@ -5143,11 +5140,8 @@ static long btrfs_ioctl_set_received_subvol_32(struct file *file,
>  	int ret = 0;
>  
>  	args32 = memdup_user(arg, sizeof(*args32));
> -	if (IS_ERR(args32)) {
> -		ret = PTR_ERR(args32);
> -		args32 = NULL;
> -		goto out;
> -	}
> +	if (IS_ERR(args32))
> +		return PTR_ERR(args32);
>  
>  	args64 = kmalloc(sizeof(*args64), GFP_NOFS);
>  	if (!args64) {
> @@ -5195,11 +5189,8 @@ static long btrfs_ioctl_set_received_subvol(struct file *file,
>  	int ret = 0;
>  
>  	sa = memdup_user(arg, sizeof(*sa));
> -	if (IS_ERR(sa)) {
> -		ret = PTR_ERR(sa);
> -		sa = NULL;
> -		goto out;
> -	}
> +	if (IS_ERR(sa))
> +		return PTR_ERR(sa);
>  
>  	ret = _btrfs_ioctl_set_received_subvol(file, sa);
>  
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 881 bytes --]