Messages in this thread | | | Date | Wed, 22 Aug 2001 11:34:39 -0700 | From | Andrew Morton <> | Subject | Re: Filling holes in ext2 |
| |
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/
| |