lkml.org 
[lkml]   [2007]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 1/9] fs: libfs buffered write leak fix
    On Fri, Feb 02, 2007 at 03:52:36PM -0800, Andrew Morton wrote:
    > On Mon, 29 Jan 2007 11:31:46 +0100 (CET)
    > Nick Piggin <npiggin@suse.de> wrote:
    >
    > > simple_prepare_write and nobh_prepare_write leak uninitialised kernel data.
    >
    > They do? Under what situation?

    Yes, I have at least reproduced the libfs leak.

    The situation is when you write into a !uptodate page, the prepare_write
    function runs SetPageUptodate *before* we have copied data in. Thus you
    can read uninitialised data out of there.

    SetPageUptodate must not be used (or at least used carefully) in
    prepare_write. commit_write is the correct place to do this.

    > > Fix the former,
    >
    > How?

    If doing a partial-write, simply clear the whole page and set it uptodate
    (don't need to get too tricky). If doing a full-write, only set it uptodate
    in the commit_write.

    > > make a note of the latter. Several other filesystems seem
    > > to be iffy here, too.
    >
    > Please, tell us what the bug is so that others have a chance of reviewing
    > and, if needed, fixing those other filesystems.
    >
    > > --- linux-2.6.orig/fs/libfs.c
    > > +++ linux-2.6/fs/libfs.c
    > > @@ -327,32 +327,35 @@ int simple_readpage(struct file *file, s
    > > int simple_prepare_write(struct file *file, struct page *page,
    > > unsigned from, unsigned to)
    > > {
    > > - if (!PageUptodate(page)) {
    > > - if (to - from != PAGE_CACHE_SIZE) {
    > > - void *kaddr = kmap_atomic(page, KM_USER0);
    > > - memset(kaddr, 0, from);
    > > - memset(kaddr + to, 0, PAGE_CACHE_SIZE - to);
    > > - flush_dcache_page(page);
    > > - kunmap_atomic(kaddr, KM_USER0);
    > > - }
    > > + if (PageUptodate(page))
    > > + return 0;
    > > +
    > > + if (to - from != PAGE_CACHE_SIZE) {
    > > + clear_highpage(page);
    > > + flush_dcache_page(page);
    > > SetPageUptodate(page);
    > > }
    >
    > memclear_highpage_flush() is fashionable.

    Good one.

    > > ===================================================================
    > > --- linux-2.6.orig/fs/buffer.c
    > > +++ linux-2.6/fs/buffer.c
    > > @@ -2344,6 +2344,8 @@ int nobh_prepare_write(struct page *page
    > >
    > > if (is_mapped_to_disk)
    > > SetPageMappedToDisk(page);
    > > +
    > > + /* XXX: information leak vs read(2) */
    > > SetPageUptodate(page);
    > >
    > > /*
    >
    > That comment is too terse to be useful.

    OK, similar problem here - we have brought all the buffers uptodate
    that we are *not* going to write over, or partially write over, but
    we can have an uninitialised hole over the region we want to write.

    I think just setting page uptodate in commit_write might do the
    trick? (and getting rid of the set_page_dirty there).
    -
    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/

    \
     
     \ /
      Last update: 2007-02-03 02:35    [W:0.031 / U:33.720 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site