Re: [PATCH] ext4: validate fiemap iomap begin offset and length value

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Murphy,


On 4/20/20 12:57 PM, Murphy Zhou wrote:
On Mon, Apr 20, 2020 at 09:46:01AM +0530, Ritesh Harjani wrote:


On 4/20/20 8:27 AM, Murphy Zhou wrote:
On Sun, Apr 19, 2020 at 09:49:27PM +0530, Ritesh Harjani wrote:
Hello Ted,

On 4/19/20 10:16 AM, Theodore Y. Ts'o wrote:

ext4_map_block() is returning EFSCORRUPTED when lblk is
EXT4_MAX_LOGICAL_BLOCK, which is why he's constraining lblk to
EXT4_MAX_LOGICAL_BLOCK.  I haven't looked into this more closely yet,

Yes, I did mention about this case in point 2 in below link though.
But maybe I was only focused on testing syzcaller reproducer, so
couldn't test this reported case.

https://www.spinics.net/lists/linux-ext4/msg71387.html


On Sun, Apr 19, 2020 at 12:42:24AM -0400, Theodore Y. Ts'o wrote:
I think we need to take his patch, and make a simialr change to
ext4_iomap_begin().   Ritesh, do you agree?

For example...

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2a4aae6acdcb..adce3339d697 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3424,8 +3424,10 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
    	int ret;
    	struct ext4_map_blocks map;
    	u8 blkbits = inode->i_blkbits;
+	ext4_lblk_t lblk = offset >> blkbits;
+	ext4_lblk_t last_lblk = (offset + length - 1) >> blkbits;

Why play with last_lblk but?



-	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
+	if (lblk > EXT4_MAX_LOGICAL_BLOCK)
    		return -EINVAL;
    	if (WARN_ON_ONCE(ext4_has_inline_data(inode)))
@@ -3434,9 +3436,15 @@ static int ext4_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
    	/*
    	 * Calculate the first and last logical blocks respectively.
    	 */
-	map.m_lblk = offset >> blkbits;
-	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
-			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
+	if (last_lblk >= EXT4_MAX_LOGICAL_BLOCK)
+		last_lblk = EXT4_MAX_LOGICAL_BLOCK - 1;
+	if (lblk >= EXT4_MAX_LOGICAL_BLOCK)
+		lblk = EXT4_MAX_LOGICAL_BLOCK - 1;
+
+	map.m_lblk = lblk;
+	map.m_len = last_lblk - lblk + 1;
+	if (map.m_len == 0 )
+		map.m_len = 1;

Not sure but with above changes map.m_len will never be
0. Right?

Yes. If it's 0, in ext4_iomap_is_delalloc we will get an "end" that
is less then m_lblk, causing another WARN in ext4_es_find_extent_range.

Sorry lost you. Ok so what I meant above is.
With your changes made in above code to truncate last_lblk
and lblk, we may never end up in a situation where map.m_len will be 0.
So the below check in your code, isn't it redundant?
I wanted to double confirm this with you.

+	if (map.m_len == 0 )
+		map.m_len = 1;

No it's not redundant. I hit and said that wo/ these two lines we will
hit a WARN later.

Ok, so thanks for the logs. I should have figured this out earlier but
duh, missed it again.

Your values are too big and your variable 'last_lblk' is of type
'ext4_lblk_t' (which is u32).
So what you maybe seeing is an overflow case where sometimes your
last_lblk is becoming just 1 less than map.m_lblk and thus your are getting map.m_len to be 0.

Can you pls carefully review and confirm now this at your end?

_(My reasoning behind was this)_
Because for m_len to become 0, we should have lblk = last_lblk + 1.
This can only happen if length passed is 0. Or last_lblk got
overflowed and become less then lblk. Now AFAIU, length passed
as argument cannot be 0.


-ritesh


At first I thought truncating values is enough, but it's not.
generic/013 (fsstress) can hit the WARN in fs/ext4/extents_status.c:266
easily.

By printk values confirmed that at that time  m_len is zero.

Found some debug notes showing how crazy these numbers are:

  offset 80000395000 length 3533d50a37ee6ddb, lblk 80000395 llblk d0a3827b
  lblk 80000395 llblk d0a3827b, m_lblk 80000395 m_len 50a37ee7
  end d0a3827b, m_lblk 80000395 m_len 50a37ee7
  offset d0a3827c000 length 3533cffffffffddb, lblk d0a3827c llblk d0a3827b
  lblk d0a3827c llblk d0a3827b, m_lblk d0a3827c m_len 0
  end d0a3827b, m_lblk d0a3827c m_len 0
  ------------[ cut here ]------------
  WARNING: CPU: 6 PID: 7962 at fs/ext4/extents_status.c:266 __es_find_extent_range+0x102/0x120 [ext4]

Thanks.




Ok, so the problem mainly is coming since ext4_map_blocks()
is returning -EFSCORRUPTED in case if lblk >= EXT4_MAX_LOGICAL_BLOCK.

So why change last_lblk?

I guess because we need to make sure a sane length value. In the loop
in iomap_fiemap, start and length are not checked, assuming be checked
by caller. If length get overflowed, the start value for the next loop
can also be affected, which makes lblk last_lblk and m_len to go crazy.

Sorry I didn't it explain it right maybe. So if we are anyway changing
lblk by truncating it and making sure map.m_len is not getting
overflowed (as we did in my previous patch), then we need not play with
last_lblk anyways.

And FWIW, instead of truncating lblk just so that ext4_map_blocks()
doesn't WARN, we can as well just return -ENOENT for
lblk >= EXT4_MAX_LOGICAL_BLOCK. ENOENT makes more sense to me,
but please feel free to correct me here.

Thoughts?

Meanwhile, I will also play this change (-ENOENT) a bit to at least get
few of the known test cases covered.


Also I do had this question for ext4.
EXT4_MAX_BLOCKS explaination says that's the max *number* of logical
blocks in a file. So since it is the number of blocks, it is equivalent
of length. Whereas the EXT4_MAX_LOGICAL_BLOCK says the max logical block
of a file, which is equivalent of offset.
Considering the logical offset starts from 0, so as Ted was saying
having both values same doesn't make sense. Ideally maybe
EXT4_MAX_LOGICAL_BLOCK should be 0xFFFFFFFFE.

But that may also require some careful checking of all bounds of length
and offset across the code. So maybe we can revisit this later.
/*
  * Maximum number of logical blocks in a file; ext4_extent's ee_block is
  * __le32.
  */
#define EXT_MAX_BLOCKS	0xffffffff


/* Max logical block we can support */
#define EXT4_MAX_LOGICAL_BLOCK		0xFFFFFFFF


-ritesh


Thanks.

Shouldn't we just change the logic to return -ENOENT in case
if (lblk >= EXT4_MAX_LOGICAL_BLOCK)? ENOENT can be handled by
IOMAP APIs to abort the loop properly.
This along with the map.m_len overlflow patch which I had submitted
before. (since the overflow patch is anyway a valid fix which we anyways
need).

-ritesh


    	if (flags & IOMAP_WRITE)
    		ret = ext4_iomap_alloc(inode, &map, flags);
@@ -3524,8 +3532,10 @@ static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
    	bool delalloc = false;
    	struct ext4_map_blocks map;
    	u8 blkbits = inode->i_blkbits;
+	ext4_lblk_t lblk = offset >> blkbits;
+	ext4_lblk_t last_lblk = (offset + length - 1) >> blkbits;
-	if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
+	if (lblk > EXT4_MAX_LOGICAL_BLOCK)
    		return -EINVAL;
    	if (ext4_has_inline_data(inode)) {
@@ -3540,9 +3550,15 @@ static int ext4_iomap_begin_report(struct inode *inode, loff_t offset,
    	/*
    	 * Calculate the first and last logical block respectively.
    	 */
-	map.m_lblk = offset >> blkbits;
-	map.m_len = min_t(loff_t, (offset + length - 1) >> blkbits,
-			  EXT4_MAX_LOGICAL_BLOCK) - map.m_lblk + 1;
+	if (last_lblk >= EXT4_MAX_LOGICAL_BLOCK)
+		last_lblk = EXT4_MAX_LOGICAL_BLOCK - 1;
+	if (lblk >= EXT4_MAX_LOGICAL_BLOCK)
+		lblk = EXT4_MAX_LOGICAL_BLOCK - 1;
+
+	map.m_lblk = lblk;
+	map.m_len = last_lblk - lblk + 1;
+	if (map.m_len == 0 )
+		map.m_len = 1;
    	/*
    	 * Fiemap callers may call for offset beyond s_bitmap_maxbytes.









[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux