Filling holes in ext2

I've been looking at generic_file_write() a lot recently, and I'm a little bothered by this section, as mangled here by Mozilla:

status = mapping->a_ops->prepare_write(file, page, offset, offset+bytes); if (status)

	goto unlock;
status = __copy_from_user(kaddr+offset, buf, bytes);
flush_dcache_page(page);
if (status)
	goto fail_write;

status = mapping->a_ops->commit_write(file, page, offset, offset+bytes);

If the __copy_from_user() does fail when writing to a hole or extending a file on ext2, disk blocks get added to the file, but are never cleared. The result is that data from a free block appears in the file.

I've not managed to trigger this in a real system, but I have explored the failure path by running UML under gdb. I filled the filesystem with data as root (yes > /mnt/test), deleted the files, then triggered this path on an application running as a normal user. The result was that root's old data appeared in the user file.

So:
Can this really happen on the mainstream kernel? (The kernel I tested on
was 2.4.7 with the corresponding UML patch.)

Can this actually be exploited? I assume the test on __copy_from_user() is there in case another thread changes memory mappings while generic_file_write() is running. My attempts to do this haven't yet succeeded.

If this can happen, does it matter? Should ext2 have an abort_write() operation like ext3() has?

Adrian Cox   http://www.humboldt.co.uk/

Yup.

Yes, it can.

I'd expect it to occur if you simply pass an unmapped address to write()?

It matters. -ac kernels handle this by clearing out the blocks on the error path in __block_prepare_write(). If you retest with -ac kernels, you should just see zeroes.

abort_write() is an expediency - it was (much) easier and safer to add a new operation rather than running all over the kernel and redefining the commit_write() API.

ext3 definitely needs to know about prepare/commit imbalance in lo_send() and generic_file_write(), and in block_symlink() if that is ever changed to error out after the prepare_write().

But long-term, we need to change the commit_write() definition so that it is called even in the error case, thus informing the underlying fs of the partial write.

-

No, because the first thing generic_file_write does is an access_ok() check. It can only happen if the permissions change during the function. That's why it's hard to exploit for real.

Adrian Cox   http://www.humboldt.co.uk/

That doesn't help. The problem occurs just the same on -ac kernels.

In this case __block_prepare_write() is successful. What happens is that if __copy_from_user() fails, the block remains mapped but not up to date. Thus the next read access to the file fetches the garbage data off disk, and presents it to the user.

The only definitive solutions I can see are:
1) implement an abort_write() method for every filesystem that uses block_prepare_write()
2) redefine commit_write() to be called on failure as well as success.
3) lock the pages corresponding to the user buffer so that __copy_from_user() cannot fail.

I like the latter option, as it removes this abort case for ext3 and could drastically simplify GFS.

Adrian Cox

Neat elegant and fundamentally close to impossible to implement without deadlocks.

There only seems to be a problem if prepare_write fills in the metadata. In such cases the fs I think does need to implement abort operations

OK, that code is for IO errors and disk-full.

generic_file_write() will mark the page not up-to-date in this case. I wonder what's actually going on? Perhaps the fact that we've instantiated a block in ext2 outside i_size?

If you change the error path in -ac's generic_file_write() thusly:

- goto fail_write;
+ status = -EFAULT;
+ goto sync_failure;

does it fix it?

Can you send the code you're using to demonstrate this?

-

The problem is that the on-disk metadata now says that that disk block is part of the file. So as the page is not up-to-date, the next read operation will go to the disk and fetch that block of garbage into the page cache.

No difference, as the write is to a hole in the middle of the file, and doesn't change i_size.

The heart of it is this:
fd = open(file, O_CREAT | O_TRUNC | O_RDWR, S_IRUSR | S_IWUSR); lseek(fd, 16385, SEEK_SET);
write(fd, "x", 1);
lseek(fd, 12345, SEEK_SET);
write(fd, "y", 1);

During the final write, I intervene with gdb to force the __copy_from_user() to fail. It should be possible to do something similar with a multi-threaded application where another thread modifies the VM, but I haven't actually built a real example.

Then I just take a look at the file with od.

[usermode:~] od -x /mnt/adrian/test
0000000 0000 0000 0000 0000 0000 0000 0000 0000
*
0030000 0a79 0a79 0a79 0a79 0a79 0a79 0a79 0a79
*
0040000 7800
0040002

Adrian Cox   http://www.humboldt.co.uk/

So in actual fact the bug is the file system committing metadata too early from prepare not commit

Alan

Ah. Now I'm with you. Yes, we need a better cleanup facility to handle this.

We can sort-of fudge it with commit_write():

@@ -2674,8 +2674,9 @@ generic_file_write(struct file *file,con
 		status = __copy_from_user(kaddr+offset, buf, bytes);
 		flush_dcache_page(page);
 		if (status) {
-			if (mapping->a_ops->abort_write)
-				mapping->a_ops->abort_write(file, page);
+			/* Zero the disk blocks so we don't expose stale data mid-file */
+			memset(kaddr + offset, 0, bytes);
+			mapping->a_ops->commit_write(file, page, offset, offset+bytes);
 			goto fail_write;
 		}
 		status = mapping->a_ops->commit_write(file, page, offset, offset+bytes);

@@ -2720,7 +2721,6 @@ out:
 fail_write:
 	status = -EFAULT;
 	ClearPageUptodate(page);
-	kunmap(page);
 	goto unlock;
 sync_failure:
 	UnlockPage(page);

Which is OK for mid-file blocks, but will cause i_size to be extended at eof, which probably isn't too bad. Needs more thought.

-

[snip]

That certainly stops it happening. Does anybody think that extending i_size in this particular corner case is harmful?

As Alan said, the bug is the file system committing metadata too early, and I suspect that ext2 is not the only culprit.

Adrian Cox   http://www.humboldt.co.uk/