linux-kernel - Re: [PATCH v2] ext4: check if offset+length is valid in fallocate

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: <39b76458-17e6-4e04-15d8-1445d2067d0c@linaro.org>
Date:   Thu, 31 Mar 2022 11:03:59 -0700
From:   Tadeusz Struk <tadeusz.struk@...aro.org>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Andreas Dilger <adilger.kernel@...ger.ca>,
        Ritesh Harjani <riteshh@...ux.ibm.com>,
        linux-ext4@...r.kernel.org, stable@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        syzbot+7a806094edd5d07ba029@...kaller.appspotmail.com
Subject: Re: [PATCH v2] ext4: check if offset+length is valid in fallocate

On 3/31/22 07:43, Theodore Ts'o wrote:
> On Tue, Mar 15, 2022 at 02:54:39PM -0700, Tadeusz Struk wrote:
>> @@ -3967,6 +3968,16 @@ int ext4_punch_hole(struct inode *inode, loff_t offset, loff_t length)
>>   		   offset;
>>   	}
>>   
>> +	/*
>> +	 * For punch hole the length + offset needs to be at least within
>> +	 * one block before last
>> +	 */
>> +	max_length = sbi->s_bitmap_maxbytes - inode->i_sb->s_blocksize;
>> +	if (offset + length >= max_length) {
>> +		ret = -ENOSPC;
>> +		goto out_mutex;
>> +	}
> 
> I wonder if we would be better off just simply capping length to
> max_length?  If length is set to some large value, such as LONG_MAX,
> it's pretty clear what the intention should be, which is to simply do
> the equivalent of truncating the file at offset.  Perhaps we should
> just do that?

Don't think that would be the correct behavior. ftrucnate (or truncate)
modify the file size, but fallocate with FALLOC_FL_PUNCH_HOLE should not.

man 2 fallocate says:
"...
The FALLOC_FL_PUNCH_HOLE flag must be ORed with FALLOC_FL_KEEP_SIZE in mode;
in other words, even when punching off the end of the file, the file size
(as reported by stat(2)) does not change.
"
that is enforced by vfs:
https://elixir.bootlin.com/linux/v5.17.1/source/fs/open.c#L245

> 
> That being said, we should be consistent with what other file systems
> do when they are asked to punch a hole starting at offset and
> extending out to LONG_MAX.

For all the supported file systems, apart from ext4, only btrfs, gfs2, and xfs
support fallocate and FALLOC_FL_PUNCH_HOLE mode.
Looking at what they do is they round the length of the space to be freed
i.e. offset + length to valid value and then perform the operation
using the valid values.

https://elixir.bootlin.com/linux/v5.17.1/source/fs/gfs2/bmap.c#L2424
https://elixir.bootlin.com/linux/v5.17.1/source/fs/btrfs/file.c#L2506

For ext4 this would mean that one could only deallocate space up to
the one before last block. I will change this to do the same in the
next version if that's ok with you.

> 
> Also, if we are going to return an error, I don't think ENOSPC is the
> correct error to be returning.

I took it from man 2 fallocate, my first suspicion was that it crashed
because the disk on my VM wasn't big enough. It was a bad choice.

-- 
Thanks,
Tadeusz