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

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: <20200419015654.F2061A4051@d06av23.portsmouth.uk.ibm.com>
Date:   Sun, 19 Apr 2020 07:26:53 +0530
From:   Ritesh Harjani <riteshh@...ux.ibm.com>
To:     Murphy Zhou <jencce.kernel@...il.com>,
        Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH] ext4: validate fiemap iomap begin offset and length value

++ mailing list.
Sorry somehow it got dropped.


On 4/19/20 7:21 AM, Ritesh Harjani wrote:
> Hello Murphy,
> 
> I guess the patch to fix this issue was recently submitted.
> Could you please test your reproducer, xfstest and ltp
> tests on below patch too. And let me know if we can add your Tested-by:
> 
> https://patchwork.ozlabs.org/project/linux-ext4/patch/1a2dc8f198e1225ddd40833de76b60c7ee20d22d.1587024137.git.riteshh@linux.ibm.com/ 
> 
> 
> -ritesh
> 
> On 4/19/20 5:02 AM, Murphy Zhou wrote:
>> Sometimes crazy userspace values can be here causing overflow issue.
>>
>> After moved ext4_fiemap to using the iomap framework in
>>    commit d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework")
>> we can hit the WARN_ON at fs/iomap/apply.c:51, then get an EIO error
>> running xfstests generic/009 (and some others) on ext4 based overlayfs.
>>
>> The minimal reproducer is:
>> -------------------------------------
>> fallocate -l 256M test.img
>> mkfs.ext4 -Fq -b 4096 -I 256 test.img
>> mkdir -p test
>> mount -o loop test.img test || exit
>> pushd test
>> rm -rf l u w m
>> mkdir -p l u w m
>> mount -t overlay -o lowerdir=l,upperdir=u,workdir=w overlay m || exit
>> xfs_io -f -c "pwrite 0 4096" -c "fiemap"  m/tf
>> umount m
>> rm -rf l u w m
>> popd
>> umount -d test
>> rm -rf test test.img
>> -------------------------------------
>>
>> Because we run fiemap command wo/ the offset and length parameters,
>> xfs_io set values based on fs blocksize etc which is got from
>> the mounted fs. These values xfs_io passed are way larger on overlayfs
>> than ext4 directly. So we can't reproduce this directly on ext4 or xfs.
>> I tried to call ioctl directly with large length value but failed to
>> reproduce this.
>>
>> I did not try to get what values xfs_io exactly passing in, but I
>> confirmed that overflowed value when it made into _ext4_fiemap.
>> It's a length of 0x7fffffffffffffff which will mess up the calculation
>> of map.m_lblk and map.m_len, make map.m_len to be 0, then hit WARN_ON
>> and get EIO in iomap_apply.
>>
>> Fixing this by ensuring the offset and length values wont exceed
>> EXT4_MAX_LOGICAL_BLOCK. Also make sure that the length would not
>> be zero because of crazy overflowed values.
>>
>> This patch has been tested with LTP/xfstests showing no new issue.
>>
>> Signed-off-by: Murphy Zhou <jencce.kernel@...il.com>
>> Fixes: d3b6f23f7167 ("ext4: move ext4_fiemap to use iomap framework")
>> ---
>>   fs/ext4/inode.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index e416096..3620417 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -3523,6 +3523,8 @@ static int ext4_iomap_begin_report(struct inode 
>> *inode, loff_t offset,
>>       int ret;
>>       bool delalloc = false;
>>       struct ext4_map_blocks map;
>> +    ext4_lblk_t last_lblk;
>> +    ext4_lblk_t lblk;
>>       u8 blkbits = inode->i_blkbits;
>>       if ((offset >> blkbits) > EXT4_MAX_LOGICAL_BLOCK)
>> @@ -3540,9 +3542,18 @@ 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;
>> +    lblk = offset >> blkbits;
>> +    last_lblk = (offset + length - 1) >> blkbits;
>> +
>> +    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.
>>