On Fri, May 12, 2017 at 12:10:04PM +0800, Shawn wrote:
>
> thanks for the reports, keep them coming . this is an interesting one,
> here's the code (same at both lines in ext4_seek_hole and ext4_seek_data):
>
> 670 ????????start = offset >> blkbits;
>
> in types this is
>
> u32 = long long >> int;
>
> since the maximum value was exceeded it means that offset (long long,
> 64 bits even on 32 bit archs) had a value that didn't fit u32 when right
> shifted. based on some light code reading, blkbits is between 10-16 on
> ext4 (EXT4_MIN_BLOCK_SIZE-EXT4_MAX_BLOCK_SIZE) so depending on what the
> actual block size of the underlying filesystem was, offset must have been
> bigger than 2^42-2^48 (4TB-256TB).
Yes, this is indeed an interesting one. I actually suspect that
offset was *negative*, and since start is a u32, this got translated
into a large number.
It is indeed a bug in ext4, although what we should do is an
interesting question, especially if I am correct that offset was in
fact negative. The man page states:
SEEK_DATA
Adjust the file offset to the next location in the file
greater than or equal to offset containing data. If
offset points to data, then the file offset is set to
offset.
(and SEEK_HOLE is similar). The man page leaves unspecified what to
do if offset is negative. We could say that this is an invalid value,
and return -EINVAL. Or we could say that since the next location in
the file greater to or equal to offset is obviously going to be a
positive number we could simply set offset to zero if it is negative.
e.g., should we add to the beginning of ext4_seek_data():
if (offset < 0)
return -EINVAL;
or
if (offset < 0)
offset = 0;
If offset really were a large number, it should have returned -ENXIO
due to this check in those functions:
isize = i_size_read(inode);
if (offset >= isize) {
inode_unlock(inode);
return -ENXIO;
}
... and we do have checks in ext4_setattr() which doesn't allow i_size
to be set to a value larger than sb->s_maxbytes. (Or
EXT4_SB(sb)->s_bitmap_maxbytes if the file is mapped using indirect
blocks.) But since we do have maxbytes handy, we might as well add a
safety check:
if (offset > maxsize)
return -ENXIO; /* or maybe -EINVAL; one could make an argument either way */
Still, it's possible for the file system to be corrupted such that the
on-disk i_size is large enough to allow offset to be insanely large.
The worse that will happen here is that we will return a bogus
location instead of returning an error, so the consequences of the
truncation are minor. But we should properly reject the call with an
error if offset is insanely large, and decide what is the proper way
to respond if SEEK_HOLE or SEEK_DATA is passed a negative offset.
Cheers,
- Ted
On 12 May 2017 at 1:02, Theodore Ts'o wrote:
[added Emese to CC and thus kept the whole mail for context even if
i'm responding to some parts of it only]
> On Fri, May 12, 2017 at 12:10:04PM +0800, Shawn wrote:
> >
> > thanks for the reports, keep them coming . this is an interesting one,
> > here's the code (same at both lines in ext4_seek_hole and ext4_seek_data):
> >
> > 670 »·······start = offset >> blkbits;
> >
> > in types this is
> >
> > u32 = long long >> int;
> >
> > since the maximum value was exceeded it means that offset (long long,
> > 64 bits even on 32 bit archs) had a value that didn't fit u32 when right
> > shifted. based on some light code reading, blkbits is between 10-16 on
> > ext4 (EXT4_MIN_BLOCK_SIZE-EXT4_MAX_BLOCK_SIZE) so depending on what the
> > actual block size of the underlying filesystem was, offset must have been
> > bigger than 2^42-2^48 (4TB-256TB).
>
> Yes, this is indeed an interesting one. I actually suspect that
> offset was *negative*, and since start is a u32, this got translated
> into a large number.
Shawn, can you do the printk instrumentation i suggested to print out
the value of offset (and isize too while at it)?
thing is, IIRC the C standard makes right shifting a negative value
implementation defined (so excluding such values would be good for that
reason alone) and i think gcc simply executes it as an arithmetic shift,
i.e., the sign of the result is preserved and thus the size overflow
checks should have detected a minimum violation, not the maximum one.
to tell for sure what check triggered exactly, i'd like to ask Shawn
to execute
make fs/ext4/file.o EXTRA_CFLAGS="-fdump-tree-all -fdump-ipa-all"
and send us (Emese in particular) all the resulting files (fs/ext4/file.*)
> It is indeed a bug in ext4, although what we should do is an
> interesting question, especially if I am correct that offset was in
> fact negative. The man page states:
>
> SEEK_DATA
> Adjust the file offset to the next location in the file
> greater than or equal to offset containing data. If
> offset points to data, then the file offset is set to
> offset.
>
> (and SEEK_HOLE is similar). The man page leaves unspecified what to
> do if offset is negative. We could say that this is an invalid value,
> and return -EINVAL. Or we could say that since the next location in
> the file greater to or equal to offset is obviously going to be a
> positive number we could simply set offset to zero if it is negative.
> e.g., should we add to the beginning of ext4_seek_data():
>
> if (offset < 0)
> return -EINVAL;
>
> or
>
> if (offset < 0)
> offset = 0;
>
> If offset really were a large number, it should have returned -ENXIO
> due to this check in those functions:
>
> isize = i_size_read(inode);
> if (offset >= isize) {
> inode_unlock(inode);
> return -ENXIO;
> }
is it possible that isize was actually big enough itself to allow a large
enough positive offset pass this test (we'll know for sure if Shawn can
get the data i asked about)? what i'm basically asking is how the blocksize
is tied to the maximum possible file size. say, can one create a >4TB file
with a blocksize of 1024 bytes, etc? by the look of it, ext4_max_bitmap_size
would allow bigger files than that.
> ... and we do have checks in ext4_setattr() which doesn't allow i_size
> to be set to a value larger than sb->s_maxbytes. (Or
> EXT4_SB(sb)->s_bitmap_maxbytes if the file is mapped using indirect
> blocks.) But since we do have maxbytes handy, we might as well add a
> safety check:
>
> if (offset > maxsize)
> return -ENXIO; /* or maybe -EINVAL; one could make an argument either way */
>
> Still, it's possible for the file system to be corrupted such that the
> on-disk i_size is large enough to allow offset to be insanely large.
>
> The worse that will happen here is that we will return a bogus
> location instead of returning an error, so the consequences of the
> truncation are minor. But we should properly reject the call with an
> error if offset is insanely large, and decide what is the proper way
> to respond if SEEK_HOLE or SEEK_DATA is passed a negative offset.
>
> Cheers,
>
> - Ted
On 05/12/2017 05:58 PM, PaX Team wrote:
> On 12 May 2017 at 1:02, Theodore Ts'o wrote:
>
> [added Emese to CC and thus kept the whole mail for context even if
> i'm responding to some parts of it only]
>
>> On Fri, May 12, 2017 at 12:10:04PM +0800, Shawn wrote:
>>>
>>> thanks for the reports, keep them coming . this is an interesting one,
>>> here's the code (same at both lines in ext4_seek_hole and ext4_seek_data):
>>>
>>> 670 »·······start = offset >> blkbits;
>>>
>>> in types this is
>>>
>>> u32 = long long >> int;
>>>
>>> since the maximum value was exceeded it means that offset (long long,
>>> 64 bits even on 32 bit archs) had a value that didn't fit u32 when right
>>> shifted. based on some light code reading, blkbits is between 10-16 on
>>> ext4 (EXT4_MIN_BLOCK_SIZE-EXT4_MAX_BLOCK_SIZE) so depending on what the
>>> actual block size of the underlying filesystem was, offset must have been
>>> bigger than 2^42-2^48 (4TB-256TB).
>>
>> Yes, this is indeed an interesting one. I actually suspect that
>> offset was *negative*, and since start is a u32, this got translated
>> into a large number.
>
> Shawn, can you do the printk instrumentation i suggested to print out
> the value of offset (and isize too while at it)?
>
I inserted the printk info in fs/ext4/file.c:677
00------------------
printk(KERN_DEBUG"offset: %lld, isize: %lld, blkbits: %d\n",
offset, isize, blkbits);
11------------------
result:
offset: 105, isize: 19595264, blkbits: 12
> thing is, IIRC the C standard makes right shifting a negative value
> implementation defined (so excluding such values would be good for that
> reason alone) and i think gcc simply executes it as an arithmetic shift,
> i.e., the sign of the result is preserved and thus the size overflow
> checks should have detected a minimum violation, not the maximum one.
>
> to tell for sure what check triggered exactly, i'd like to ask Shawn
> to execute
>
> make fs/ext4/file.o EXTRA_CFLAGS="-fdump-tree-all -fdump-ipa-all"
>
> and send us (Emese in particular) all the resulting files (fs/ext4/file.*)
>
find the file here:
https://ufile.io/9ie40
Let me know if you need anything else.
regards
Shawn
On 13 May 2017 at 12:23, Shawn C wrote:
> On 05/12/2017 05:58 PM, PaX Team wrote:
> > Shawn, can you do the printk instrumentation i suggested to print out
> > the value of offset (and isize too while at it)?
> >
> I inserted the printk info in fs/ext4/file.c:677
>
> 00------------------
> printk(KERN_DEBUG"offset: %lld, isize: %lld, blkbits: %d\n",
> offset, isize, blkbits);
> 11------------------
>
> result:
>
> offset: 105, isize: 19595264, blkbits: 12
are you sure this is the last message before the size overflow report?
can you perhaps post the relevant portion of dmesg too?