linux-ext4 - Re: strange ext{3,4}

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080316113912.GA3118@dmon-lap.sw.ru>
Date:	Sun, 16 Mar 2008 14:39:12 +0300
From:	Dmitri Monakhov <dmonakhov@...nvz.org>
To:	Andreas Dilger <adilger@....com>
Cc:	linux-ext4@...r.kernel.org, Jens Axboe <jens.axboe@...cle.com>
Subject: Re: strange ext{3,4}_settattr logic

I've added Jens because he may be also interesting in this topic.
On 08:23 Sun 16 Mar     , Andreas Dilger wrote:
> On Mar 16, 2008  07:54 +0800, Andreas Dilger wrote:
> > A call to  inode_setattr() can fail by trying a shrinking vmtruncate()
> > on a swapfile, which returns ETXTBUSY.  This was added after the
> > ext3_setattr() code was written.
> > 
> > We need to handle the IS_SWAPFILE() case properly.
> > Granted, it probably isn't a very common problem, but the IS_SWAPFILE()
> > check was added explicitly because of clueless users, so it must be hit
> > occasionally in real life.
> > 
> > It would seem that if you have a swapfile, try to truncate it to 0 (which
> > will fail with -ETXTBUSY) and then unmount the filesystem the size will
> > be truncated to 0.  It is also possible to directly write to a swapfile
> > and corrupt memory, or read from a swapfile and access potentially sensitive
> > information.
In fact i've triggered this issue while working on fast_loop device
implementation which was proposed by Jens (http://lkml.org/lkml/2008/1/9/50).
In fast_loop device use swapfile approach (submitting bio-s directly to
underlying block device). I think this idea will be included in mainstream
loop device sooner or later. But approach has several issues:
One of the most important is effective control mechanism over truncates for
lower file,  this issue was missed in Jens patch set.
This mechanism probably have to have following options.
#1: Shrink truncates must be denied.
#2: Expand truncates may be allowed. This is good because most of non plain 
disk image formats (qcow, vmdk, and etc) are growing while adding new data
blocks.
#3: Allow exclusive owner for file, for example only one user(loop_thread in
this case) may truncate file. Provide something similar to bd_claim feature.
without this feature on-line shrinking of disk image looks like this:

 mutex_lock(&inode->i_mutex);
 inode->i_flags &= ~S_SWAPFILE;
 mutex_unlock(&inode->i_mutex);

 /* Perform shrinking truncate. This is absolutely racy operation because
  * some one else also may perform truncate at this time*/
 do_truncate(inode, size); 

 mutex_lock(&inode->i_mutex);
 inode->i_flags |= S_SWAPFILE;
 mutex_unlock(&inode->i_mutex);


S_SWAPFILE inode option currently equals to #1, and #2. What's why i want
use this flag for fast_loop devices.

> 
> Dmitri, (or anyone)
> can you please give this patch a try.  It should be OK, but I'm hesitant
> to test it on my test box because I'm out of town and if it fails to
> reboot I can't fix it for a week.  It _should_ still be possible to chown,
> rename, ls, etc the swapfile, can you please verify that in addition
> to the simple test in my previous email.
> 
[snip]
> 
> 
> 
> --- linux-2.6.24/fs/namei.c.orig	2008-02-05 07:29:57.000000000 +0800
> +++ linux-2.6.24/fs/namei.c	2008-03-16 08:11:41.000000000 +0800
> @@ -233,6 +233,10 @@ int permission(struct inode *inode, int 
>  	if (nd)
>  		mnt = nd->mnt;
>  
> +	/* Don't allow direct read, write, or truncate on a swapfile */
Your patch disallow expand truncates (#2) which is definitely not good. 
In fact if file was opened before S_SWAPFILE flag was setted when it is
possible to directly read, write from file.
> +	if (IS_SWAPFILE(inode))
> +		return -ETXTBUSY;
> +
>  	if (mask & MAY_WRITE) {
>  		umode_t mode = inode->i_mode;

BTW it is impossible to disable swapfile with your patch 
[root@...3 tmp]# swapoff  /vz/swap 
swapoff: /vz/swap: Text file busy

IMHO S_SWAPFILE flag must be checked in ext3_setattr, see patch attached:

View attachment "swap_flag.patch" of type "text/plain" (464 bytes)