[PATCH] isofs: mounting to regular file may succeed

[PATCH] isofs: mounting to regular file may succeed

[Kirill Kuvaldin]

It turned out that mounting a corrupted ISO image to a regular file may
succeed, e.g. if an image was prepared as follows:

$ dd if=correct.iso of=bad.iso bs=4k count=8

We then can mount it to a regular file:

# mount -o loop -t iso9660 bad.iso /tmp/file

But mounting it to a directory fails with -ENOTDIR, simply because 
the root directory inode doesn't have S_IFDIR set and the condition
in graft_tree() is met:

	if (S_ISDIR(nd->dentry->d_inode->i_mode) !=
	      S_ISDIR(mnt->mnt_root->d_inode->i_mode))
		return -ENOTDIR

This is because the root directory inode was read from an incorrect
block. It's supposed to be read from sbi->s_firstdatazone, which is
an absolute value and gets messed up in the case of an incorrect image.

In order to somehow circumvent this we have to check that the root
directory inode is actually a directory after all.


Signed-off-by: Kirill Kuvaldin <kuvkir@epsmu.com>

diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
index 5c3eecf..ce5062a 100644
--- a/fs/isofs/inode.c
+++ b/fs/isofs/inode.c
@@ -840,6 +840,15 @@ root_found:
 		goto out_no_root;
 	if (!inode->i_op)
 		goto out_bad_root;
+
+	/* Make sure the root inode is a directory */
+	if (!S_ISDIR(inode->i_mode)) {
+		printk(KERN_WARNING
+			"isofs_fill_super: root inode is not a directory. "
+			"Corrupted media?\n");
+		goto out_iput;
+	}
+
 	/* get the root dentry */
 	s->s_root = d_alloc_root(inode);
 	if (!(s->s_root))

Re: [PATCH] isofs: mounting to regular file may succeed

[Kirill Kuvaldin]

On Sat, Jul 14, 2007 at 03:47:21AM +0400, Kirill Kuvaldin wrote:
> $ dd if=correct.iso of=bad.iso bs=4k count=8

Oops, sorry, the right command should be:

dd if=correct.iso of=bad.iso bs=4k seek=8
                                   ^^^^
 

Kirill

Re: [PATCH] isofs: mounting to regular file may succeed

[Jan Engelhardt]

On Jul 14 2007 03:47, Kirill Kuvaldin wrote:
>
>We then can mount it to a regular file:

Wow, this is news to me. Since when is it possible to mount files to files?

	Jan
-- 

Re: [PATCH] isofs: mounting to regular file may succeed

[Kirill Kuvaldin]

On Sat, Jul 14, 2007 at 09:16:51PM +0200, Jan Engelhardt wrote:
> 
> On Jul 14 2007 03:47, Kirill Kuvaldin wrote:
> >
> >We then can mount it to a regular file:
> 
> Wow, this is news to me. Since when is it possible to mount files to files?
> 

It is possible to mount a regular file to another one with --bind.
The problem in question is that mounting a malformed ISO 9660 image to a
directory fails, but to a regular file - succeeds.

Kirill

Re: [PATCH] isofs: mounting to regular file may succeed

[Andrew Morton]

On Sat, 14 Jul 2007 03:47:21 +0400 Kirill Kuvaldin <kuvkir@epsmu.com> wrote:

> It turned out that mounting a corrupted ISO image to a regular file may
> succeed, e.g. if an image was prepared as follows:
> 
> $ dd if=correct.iso of=bad.iso bs=4k count=8
> 
> We then can mount it to a regular file:
> 
> # mount -o loop -t iso9660 bad.iso /tmp/file
> 
> But mounting it to a directory fails with -ENOTDIR, simply because 
> the root directory inode doesn't have S_IFDIR set and the condition
> in graft_tree() is met:
> 
> 	if (S_ISDIR(nd->dentry->d_inode->i_mode) !=
> 	      S_ISDIR(mnt->mnt_root->d_inode->i_mode))
> 		return -ENOTDIR
> 
> This is because the root directory inode was read from an incorrect
> block. It's supposed to be read from sbi->s_firstdatazone, which is
> an absolute value and gets messed up in the case of an incorrect image.
> 
> In order to somehow circumvent this we have to check that the root
> directory inode is actually a directory after all.
> 
> 
> Signed-off-by: Kirill Kuvaldin <kuvkir@epsmu.com>
> 
> diff --git a/fs/isofs/inode.c b/fs/isofs/inode.c
> index 5c3eecf..ce5062a 100644
> --- a/fs/isofs/inode.c
> +++ b/fs/isofs/inode.c
> @@ -840,6 +840,15 @@ root_found:
>  		goto out_no_root;
>  	if (!inode->i_op)
>  		goto out_bad_root;
> +
> +	/* Make sure the root inode is a directory */
> +	if (!S_ISDIR(inode->i_mode)) {
> +		printk(KERN_WARNING
> +			"isofs_fill_super: root inode is not a directory. "
> +			"Corrupted media?\n");
> +		goto out_iput;
> +	}
> +
>  	/* get the root dentry */
>  	s->s_root = d_alloc_root(inode);
>  	if (!(s->s_root))

I don't think any (all?) other filesystems perform checks like this.
Is this something which can/should be performed at the VFS level?

Re: [PATCH] isofs: mounting to regular file may succeed

[Christoph Hellwig]

On Tue, Jul 17, 2007 at 12:04:07AM -0700, Andrew Morton wrote:
> I don't think any (all?) other filesystems perform checks like this.
> Is this something which can/should be performed at the VFS level?

As far as the VFS is concerned non-directory mounts are perfectly fine.
There's a lot of use cases for non-directory bind-mounts and at least
some for regular filesystems with a non-directory root.  E.g. the streams
folks are using something like that.    Solaris even ships with non-directory
root filesystems mounted by default these days.

Re: [PATCH] isofs: mounting to regular file may succeed

[Al Viro]

On Tue, Jul 17, 2007 at 12:04:07AM -0700, Andrew Morton wrote:

> I don't think any (all?) other filesystems perform checks like this.
> Is this something which can/should be performed at the VFS level?

	I don't see why we _should_ check that; VFS checks that we don't
turn directory into non-directory and vice versa, same as for binding.
There is no reason why fs driver could not give you a single-node filesystem
with root being e.g. a block device node.  You could mount it on any
non-directory and get a block device seen there, etc.

	IOW, if filesystem driver considers fs image it's working from
as invalid, it's up to filesystem driver.  _What_ is considered invalid
depends on fs type; what the driver cares to report as dubious is, again,
up to driver.  If isofs wants to warn about an ISO image that definitely
violates ISO9660, it's isofs decision.  VFS will work with it as long
as fs driver does...