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

Path	csiph.com!news.redatomik.org!aioe.org!gothmog.csi.it!bofh.it!news.nic.it!robomod
From	Jeff Mahoney <jeffm@suse.com>
Newsgroups	linux.kernel
Subject	Re: Fs: Btrfs - Fix possible ERR_PTR() dereferencing.
Date	Tue, 20 Sep 2016 15:10:01 +0200
Message-ID	<sjsY9-3nb-7@gated-at.bofh.it> (permalink)
References	<sjn2p-7SH-7@gated-at.bofh.it>
User-Agent	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0) Gecko/20100101 Thunderbird/45.3.0
MIME-Version	1.0
Content-Type	multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="nR2Hi0wAWAMg0GeRjnoBhu93s7qLFQif8"
Sender	robomod@news.nic.it
List-ID	<linux-kernel.vger.kernel.org>
X-Mailing-List	linux-kernel@vger.kernel.org
Approved	robomod@news.nic.it
Lines	134
Organization	linux.* mail to news gateway
X-Original-Cc	linux-kernel@vger.kernel.org, vidushi.koul@samsung.com
X-Original-Date	Tue, 20 Sep 2016 09:00:44 -0400
X-Original-Message-ID	<7050d410-dbf1-15a3-a6ba-2ae28f1fb0ee@suse.com>
X-Original-References	<1474354107-18774-1-git-send-email-shailendra.v@samsung.com>
X-Original-Sender	linux-kernel-owner@vger.kernel.org
Xref	csiph.com linux.kernel:1487318

Show key headers only | View raw

[Multipart message — attachments visible in raw view] - view raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

Fs: Btrfs - Fix possible ERR_PTR() dereferencing. Shailendra Verma <shailendra.v@samsung.com> - 2016-09-20 08:50 +0200
  Re: Fs: Btrfs - Fix possible ERR_PTR() dereferencing. Jeff Mahoney <jeffm@suse.com> - 2016-09-20 15:10 +0200

csiph-web