Re: [PATCH] libxfs: don't write uninitialized heap contents into new directory blocks

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, Mar 20, 2015 at 09:59:17AM +1100, Dave Chinner wrote:
> On Thu, Mar 19, 2015 at 02:02:50PM -0700, Darrick J. Wong wrote:
> > On Thu, Mar 19, 2015 at 02:28:34PM -0400, Brian Foster wrote:
> > > On Wed, Mar 18, 2015 at 04:21:59PM -0700, Darrick J. Wong wrote:
> > > > When we're initializing a new directory block, zero the buffer
> > > > contents to avoid writing random heap contents (and crc thereof)
> > > > to disk.  This eliminates a few valgrind complaints in xfs_repair.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > 
> > > Well it seems Ok to me:
> > > 
> > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > 
> > > ... but I do notice that we start ending up with multiple memset() calls
> > > throughout these codepaths. E.g., xfs_dir3_data_init() memsets the
> > > target bytes for the header if crc is enabled, xfs_dir3_leaf_get_buf()
> > > calls xfs_dir3_leaf_init() which does something similar, etc.
> > > 
> > > I wonder if we should just zero the buffer content unconditionally on
> > > allocation?
> > 
> > I thought about that too -- certainly we could zero b_addr as soon as it's
> > allocated in __initbuf, which would shut up Valgrind.  I think this is what
> > Dave was getting at when he suggested __libxfs_getbufr(), even though that
> > function only frees b_addr if it touches it at all.
> 
> It frees b_addr only if the buffer found on the free list does not
> match the length of the new buffer. Otherwise it just reuses it
> directly. It probably doesn't matter if zeroing is done in
> __initbuf, it is always called after __libxfs_getbufr() returns a
> buffer handle, anyway.

Right.  I'll do it there, since __initbuf exists only outside of the kernel.

> > On the other hand, the
> > next thing could happen is that the buffer is read in from disk, which makes
> > the zeroing unnecessary.  The readbuf functions call the getbuf functions,
> > which makes it difficult to tell from the getbuf functions what's going to
> > happen next.
> > 
> > However, I think there's a second problem: what if the xfsbuf is reused without
> > freeing the b_addr?
> 
> Yup, that's what __libxfs_getbufr() does ;)
> 
> > This seems possible if we read in the block, detach it
> > from whatever points to it, and then we want to allocate it to a directory.
> > The buffer's still in memory and it's still the right size, so we don't free
> > b_addr; the init function doesn't overwrite the whole buffer, and now we've
> > just leaked old disk contents.  Adding a memset to getbuf would fix this, but
> > again at a cost of unnecessary zeroing.
> 
> Leaked the contents to whom?
> 
> In the kernel we don't care about stale contents in metadata buffers
> as the contents cannot be directly read from userspace. i.e. there
> is no vector for information leakage (other than through the root
> user reading the bdev directly), so we don't need to zero buffers to
> prevent information leakage either in userspace or the kernel...

Ok.  It was a mostly theoretical worry -- a corrupt FS thinks a data block
contains metadata; repair frees the block but then reallocates it to a dir
or something, and then a second file gets pointed to the dir block.  Now 
the second file can read the old file's contents, even after we supposedly
reallocated it to something else.

OTOH it's probably hard to make that happen.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs