Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1534974222.2902.15.camel@codethink.co.uk>
Subject: Re: [PATCH 4.4 40/43] loop: add recursion validation to
 LOOP_CHANGE_FD
From:   Ben Hutchings <ben.hutchings@codethink.co.uk>
To:     Theodore Tso <tytso@mit.edu>, Jens Axboe <axboe@kernel.dk>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>, stable@vger.kernel.org
Date:   Wed, 22 Aug 2018 22:43:42 +0100
In-Reply-To: <20180716073516.296730239@linuxfoundation.org>
References: <20180716073511.796555857@linuxfoundation.org>
         <20180716073516.296730239@linuxfoundation.org>
Organization: Codethink Ltd.
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 2018-07-16 at 09:36 +0200, Greg Kroah-Hartman wrote:
> 4.4-stable review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Theodore Ts'o <tytso@mit.edu>
> 
> commit d2ac838e4cd7e5e9891ecc094d626734b0245c99 upstream.
> 
> Refactor the validation code used in LOOP_SET_FD so it is also used in
> LOOP_CHANGE_FD.  Otherwise it is possible to construct a set of loop
> devices that all refer to each other.  This can lead to a infinite
> loop in starting with "while (is_loop_device(f)) .." in loop_set_fd().
> 
> Fix this by refactoring out the validation code and using it for
> LOOP_CHANGE_FD as well as LOOP_SET_FD.
[...]
> +static int loop_validate_file(struct file *file, struct block_device *bdev)
> +{
> +	struct inode	*inode = file->f_mapping->host;
> +	struct file	*f = file;
> +
> +	/* Avoid recursion */
> +	while (is_loop_device(f)) {
> +		struct loop_device *l;
> +
> +		if (f->f_mapping->host->i_bdev == bdev)
> +			return -EBADF;
> +
> +		l = f->f_mapping->host->i_bdev->bd_disk->private_data;
> +		if (l->lo_state == Lo_unbound) {
> +			return -EINVAL;
> +		}
> +		f = l->lo_backing_file;

This looks racy; I don't see anything that prevents a lower loop device
from being reconfigured while this walks down the device stack.

(But this isn't a new problem.)

Ben.

> +	}
> +	if (!S_ISREG(inode->i_mode) && !S_ISBLK(inode->i_mode))
> +		return -EINVAL;
> +	return 0;
> +}
[...]

-- 
Ben Hutchings, Software Developer                         Codethink Ltd
https://www.codethink.co.uk/                 Dale House, 35 Dale Street
                                     Manchester, M1 2HF, United Kingdom