On Tue, Jul 12, 2022 at 5:48 PM Darrick J. Wong <djwong@xxxxxxxxxx> wrote:
>
> AFAICT, the reason why dedupe does all the weird checks it does is
> because apparently some of the dedupe tools expect that they can do
> weird things like dedupe into a file that they own but haven't opened
> for writes or could open for writes. Change those bits, and you break
> userspace.
Christ, what a crock.
> The dedupe implementations /do/ check file blocksize, it's under
> generic_remap_file_range_prep. The reason this exploit program works on
> the 7-byte file is that we stop comparing at EOF, which means that there
> are fewer bytes to guess. But. You can already read the file anyway.
The dedupe clearly does *not* check for blocksize. It doesn't even
call generic_remap_file_range_prep().
Just check it yourself:
do_vfs_ioctl ->
case FIDEDUPERANGE:
ioctl_file_dedupe_range(filp, argp) ->
vfs_dedupe_file_range(file, same) ->
and there it is. All that calls is remap_verify_area() for both files,
and allow_file_dedupe() for the target.
Now, maybe some filesystem then calls the checking functions, but
considering the posted code, that can't be true, because no, the the
whole EOF argument is garbage, because even if the *source* was at
EOF, the destination offset should still have been checked for being
at a block boundary.
And it clearly wasn't, since the code just walks the destination
offset one byte at a time.
So you may *think* that it checks the file blocksize, but I'm afraid
reality disagrees. And no amount of "source is at EOF" is possibly
relevant for the destination block size check.
> So we're going to break userspace?
> https://github.com/markfasheh/duperemove/issues/129
Well, if you're supposed to be able to dedupe read-only files, then
the whole "check for writing" is just bogus to begin with.
So in no way are any of those checks possibly correct.
The destination offset is clearly not checked at all.
And if writability isn't something you want honored, then FMODE_WRITE
shouldn't have been an issue.
> ...and we're going to break deduping the EOF block again?
Why are you arguing about something that is clearly broken.
The lack of destination offset checking cannot possibly be rigth. No
amount of "EOF block" is relevant AT ALL.
Stop it. You're making an argument that has nothing to do with the bug at hand.
> What /is/ the meaning of S_IMMUTABLE? Documentation/ only says that the
> fsverity author thinks it means "read-only contents". If it's the same
> as "chmod 0000" in that anyone with a writable fd can still modify the
> supposedly immutable file, then what was the point?
No, the whole point is that you cannot ever get a writable file
descriptor to an immutable file.
So if that FMODE_WRITE check is correct, then IS_IMMUTABLE is nonsense.
And if MODE_WRITE isn't correct, and dedupe is considered a read-only
operation, then *that* isn't valid.
You can't have it both ways. Which is it?
That's really my point here - all those checks are completely random
noise. There is absoltuely no rhyme or reason to them.
And the offset check is clearly not there, and you should stop talking
about "source EOF", because that is irrelevant. Source EOF may affect
the *length* of the block, but it does not affect the offset of the
destination.
Linus
