linux-ext4 - Re: [PATCH] Prevent creation of files larger than RLIMIT

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: <4BD85F1F.7030100@suse.de>
Date:	Thu, 29 Apr 2010 00:15:27 +0800
From:	Coly Li <coly.li@...e.de>
To:	Nikanth Karthikesan <knikanth@...e.de>
Cc:	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org, Theodore Ts'o <tytso@....edu>,
	Andreas Dilger <adilger@....com>, linux-ext4@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Eelis <opensuse.org@...tacts.eelis.net>
Subject: Re: [PATCH] Prevent creation of files larger than RLIMIT_FSIZE using
 fallocate



On 04/28/2010 09:24 PM, Nikanth Karthikesan Wrote:
> Prevent creation of files larger than RLIMIT_FSIZE using fallocate.
> 
> Currently using posix_fallocate one can bypass an RLIMIT_FSIZE limit
> and create a file larger than the limit. Add a check for new size in
> the fallocate system call.
> 
> File-systems supporting fallocate such as ext4 are affected by this
> bug. 
> 
> Signed-off-by: Nikanth Karthikesan <knikanth@...e.de>
> Reported-by: Eelis - <opensuse.org@...tacts.eelis.net>
> 
> ---
> 
> diff --git a/fs/open.c b/fs/open.c
> index 74e5cd9..95ce069 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -412,10 +412,14 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>  	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
>  		return -ENODEV;
>  
> -	/* Check for wrap through zero too */
> -	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
> +	/* Check for wrap through zero */
> +	if (offset+len < 0)
>  		return -EFBIG;
>  
> +	ret = inode_newsize_ok(inode, (offset + len));
> +	if (ret)
> +		return ret;
> +
>  	if (!inode->i_op->fallocate)
>  		return -EOPNOTSUPP;
>  

Hi Nikanth,

>From definition of inode_newsize_ok(), it says,
 64 /**
 65  * inode_newsize_ok - may this inode be truncated to a given size
 66  * @inode:      the inode to be truncated
 67  * @offset:     the new size to assign to the inode
 68  * @Returns:    0 on success, -ve errno on failure
 69  *
 70  * inode_newsize_ok will check filesystem limits and ulimits to check that the
 71  * new inode size is within limits. inode_newsize_ok will also send SIGXFSZ
 72  * when necessary. Caller must not proceed with inode size change if failure is
 73  * returned. @inode must be a file (not directory), with appropriate
 74  * permissions to allow truncate (inode_newsize_ok does NOT check these
 75  * conditions).
 76  *
 77  * inode_newsize_ok must be called with i_mutex held.
 78  */

In execution path of do_fallocate(), it seems no i_mutex held, and inode might be directory. Using inode_newsize_ok()
might be confused ?

How about this one ?

 fs/open.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 74e5cd9..24d75a4 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -383,6 +383,7 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftruncate64);
 int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
 	struct inode *inode = file->f_path.dentry->d_inode;
+	unsigned long limit;
 	long ret;

 	if (offset < 0 || len <= 0)
@@ -412,10 +413,19 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
 		return -ENODEV;

-	/* Check for wrap through zero too */
-	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
+	/* Check for wrap through zero */
+	if ((offset + len) < 0)
 		return -EFBIG;

+	/* Check for rlimit */
+	if ((offset + len) > inode->i_size) {
+		limit = rlimit(RLIMIT_FSIZE);
+		if (limit != RLIM_INFINITY && (offset + len) > limit)
+			return -EFBIG;
+		if ((offset + len) > inode->i_sb->s_maxbytes)
+			return -EFBIG;
+	}
+
 	if (!inode->i_op->fallocate)
 		return -EOPNOTSUPP;

Something I don't understand here is, why no i_mutex hold here ?

-- 
Coly Li
SuSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html