Adrian Cox wrote:
>
> 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.
Yup.
> 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.)
Yes, it can.
> 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.
I'd expect it to occur if you simply pass an unmapped address
to write()?
> If this can happen, does it matter?
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.
> Should ext2 have an abort_write() operation like ext3() has?
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.
-
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Thu Aug 23 2001 - 21:00:50 EST