linux-kernel - Re: [PATCH v8 1/9] dax: fix NULL pointer dereference in __dax

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160113070829.GA30496@linux.intel.com>
Date:	Wed, 13 Jan 2016 00:08:29 -0700
From:	Ross Zwisler <ross.zwisler@...ux.intel.com>
To:	Jan Kara <jack@...e.cz>
Cc:	Ross Zwisler <ross.zwisler@...ux.intel.com>,
	linux-kernel@...r.kernel.org, "H. Peter Anvin" <hpa@...or.com>,
	"J. Bruce Fields" <bfields@...ldses.org>,
	Theodore Ts'o <tytso@....edu>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Andreas Dilger <adilger.kernel@...ger.ca>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dan Williams <dan.j.williams@...el.com>,
	Dave Chinner <david@...morbit.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Ingo Molnar <mingo@...hat.com>, Jan Kara <jack@...e.com>,
	Jeff Layton <jlayton@...chiereds.net>,
	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	Matthew Wilcox <willy@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-mm@...ck.org, linux-nvdimm@...ts.01.org, x86@...nel.org,
	xfs@....sgi.com
Subject: Re: [PATCH v8 1/9] dax: fix NULL pointer dereference in __dax_dbg()

On Tue, Jan 12, 2016 at 10:34:58AM +0100, Jan Kara wrote:
> On Thu 07-01-16 22:27:51, Ross Zwisler wrote:
> > In __dax_pmd_fault() we currently assume that get_block() will always set
> > bh.b_bdev and we unconditionally dereference it in __dax_dbg().  This
> > assumption isn't always true - when called for reads of holes
> > ext4_dax_mmap_get_block() returns a buffer head where bh->b_bdev is never
> > set.  I hit this BUG while testing the DAX PMD fault path.
> > 
> > Instead, initialize bh.b_bdev before passing bh into get_block().  It is
> > possible that the filesystem's get_block() will update bh.b_bdev, and this
> > is fine - we just want to initialize bh.b_bdev to something reasonable so
> > that the calls to __dax_dbg() work and print something useful.
> > 
> > Signed-off-by: Ross Zwisler <ross.zwisler@...ux.intel.com>
> > Cc: Dan Williams <dan.j.williams@...el.com>
> 
> Looks good. But don't you need to do the same for __dax_fault(),
> dax_zero_page_range() and similar places passing bh to dax functions?
> 
> 								Honza

I don't think we need it anywhere else.  The only reason that we need to
initialize the bh.b_bdev manually in the __dax_pmd_fault() path is that if the
get_block() call ends up finding a hole (so doesn't fill out b_bdev) we still
go through the dax_pmd_dbg() path to print an error message, which uses
b_bdev.  I believe that in the other paths where we hit a hole, such as
__dax_fault(), we don't use b_bdev because we don't have the same error path
prints, and the regular code for handling holes doesn't use b_bdev.

That being said, if you feel like it's cleaner to initialize it everywhere so
everything is consistent and we don't have to worry about it, I'm fine to make
the change.

> > ---
> >  fs/dax.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 7af8797..513bba5 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -624,6 +624,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address,
> >  	}
> >  
> >  	memset(&bh, 0, sizeof(bh));
> > +	bh.b_bdev = inode->i_sb->s_bdev;
> >  	block = (sector_t)pgoff << (PAGE_SHIFT - blkbits);
> >  
> >  	bh.b_size = PMD_SIZE;
> > -- 
> > 2.5.0
> > 
> > 
> -- 
> Jan Kara <jack@...e.com>
> SUSE Labs, CR