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

Date:	Fri, 30 Apr 2010 14:33:19 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Nikanth Karthikesan <knikanth@...e.de>
Cc:	coly.li@...e.de, Nick Piggin <npiggin@...e.de>,
	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,
	Eelis <opensuse.org@...tacts.eelis.net>,
	Amit Arora <aarora@...ibm.com>
Subject: Re: [PATCH] Prevent creation of files larger than RLIMIT_FSIZE
 using fallocate


(Amit Arora <aarora@...ibm.com> wrote fallocate.  cc added)

On Thu, 29 Apr 2010 10:14:06 +0530
Nikanth Karthikesan <knikanth@...e.de> wrote:

> Here is an updated patch that takes the i_mutex and calls inode_newsize_ok()
> only for regular files.

err, no.  It's taking i_lock where it meant to take i_mutex.

> Thanks
> Nikanth
> 
> 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..4ca57c9 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -405,17 +405,26 @@ int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>  	if (S_ISFIFO(inode->i_mode))
>  		return -ESPIPE;
>  
> -	/*
> -	 * Let individual file system decide if it supports preallocation
> -	 * for directories or not.
> -	 */
> -	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;

I suggest that this test be moved up to where the function tests `if
(offset < 0 || len <= 0)' - it seems more logical.

Also,

-	if (offset+len < 0)
+	if (offset + len < 0)

for consistency with most other kernel code, please.

> +	if (S_ISREG(inode->i_mode)) {
> +		spin_lock(&inode->i_lock);
> +		ret = inode_newsize_ok(inode, (offset + len));
> +		spin_unlock(&inode->i_lock);
> +		if (ret)
> +			return ret;
> +	} else if (S_ISDIR(inode->i_mode)) {
> +		/*
> +		 * Let individual file system decide if it supports
> +		 * preallocation for directories or not.
> +		 */
> +		if (offset > inode->i_sb->s_maxbytes)
> +			return -EFBIG;
> +	} else
> +		return -ENODEV;
> +
>  	if (!inode->i_op->fallocate)
>  		return -EOPNOTSUPP;

Also, there doesn't seem to be much point in doing

	mutex_lock(i_mutex);
	if (some_condition)
		bale out
	mutex_unlock(i_mutex);

	<stuff>

because `some_condition' can now become true before or during the
execution of `stuff'.

IOW, it's racy.
--
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