linux-kernel - Re: Fs: Btrfs - Fix possible ERR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7050d410-dbf1-15a3-a6ba-2ae28f1fb0ee@suse.com>
Date:   Tue, 20 Sep 2016 09:00:44 -0400
From:   Jeff Mahoney <jeffm@...e.com>
To:     Shailendra Verma <shailendra.v@...sung.com>,
        Chris Mason <clm@...com>, Josef Bacik <jbacik@...com>,
        David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
        Ravikant Sharma <ravikant.s2@...sung.com>
Cc:     linux-kernel@...r.kernel.org, vidushi.koul@...sung.com
Subject: Re: Fs: Btrfs - Fix possible ERR_PTR() dereferencing.

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@...sung.com>
> Signed-off-by: Shailendra Verma <shailendra.v@...sung.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



Download attachment "signature.asc" of type "application/pgp-signature" (882 bytes)