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

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

[Darrick J. Wong]

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>
---
 libxfs/xfs_da_btree.c  |    1 +
 libxfs/xfs_dir2_data.c |    1 +
 libxfs/xfs_dir2_leaf.c |    1 +
 libxfs/xfs_dir2_node.c |    2 +-
 4 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/libxfs/xfs_da_btree.c b/libxfs/xfs_da_btree.c
index b731b54..bc73cbc 100644
--- a/libxfs/xfs_da_btree.c
+++ b/libxfs/xfs_da_btree.c
@@ -562,6 +562,7 @@ xfs_da3_root_split(
 	if (error)
 		return error;
 	node = bp->b_addr;
+	memset(bp->b_addr, 0, bp->b_bcount);
 	oldroot = blk1->bp->b_addr;
 	if (oldroot->hdr.info.magic == cpu_to_be16(XFS_DA_NODE_MAGIC) ||
 	    oldroot->hdr.info.magic == cpu_to_be16(XFS_DA3_NODE_MAGIC)) {
diff --git a/libxfs/xfs_dir2_data.c b/libxfs/xfs_dir2_data.c
index dc9df4d..79c2fc2 100644
--- a/libxfs/xfs_dir2_data.c
+++ b/libxfs/xfs_dir2_data.c
@@ -559,6 +559,7 @@ xfs_dir3_data_init(
 		XFS_DATA_FORK);
 	if (error)
 		return error;
+	memset(bp->b_addr, 0, bp->b_bcount);
 	bp->b_ops = &xfs_dir3_data_buf_ops;
 	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DIR_DATA_BUF);
 
diff --git a/libxfs/xfs_dir2_leaf.c b/libxfs/xfs_dir2_leaf.c
index 8e0cbc9..6eb03fc 100644
--- a/libxfs/xfs_dir2_leaf.c
+++ b/libxfs/xfs_dir2_leaf.c
@@ -382,6 +382,7 @@ xfs_dir3_leaf_get_buf(
 	if (error)
 		return error;
 
+	memset(bp->b_addr, 0, bp->b_bcount);
 	xfs_dir3_leaf_init(mp, tp, bp, dp->i_ino, magic);
 	xfs_dir3_leaf_log_header(tp, bp);
 	if (magic == XFS_DIR2_LEAF1_MAGIC)
diff --git a/libxfs/xfs_dir2_node.c b/libxfs/xfs_dir2_node.c
index 3737e4e..3e2f04d 100644
--- a/libxfs/xfs_dir2_node.c
+++ b/libxfs/xfs_dir2_node.c
@@ -247,7 +247,7 @@ xfs_dir3_free_get_buf(
 	 * Initialize the new block to be empty, and remember
 	 * its first slot as our empty slot.
 	 */
-	memset(bp->b_addr, 0, sizeof(struct xfs_dir3_free_hdr));
+	memset(bp->b_addr, 0, bp->b_bcount);
 	memset(&hdr, 0, sizeof(hdr));
 
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {

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

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

[Brian Foster]

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?

Brian

>  libxfs/xfs_da_btree.c  |    1 +
>  libxfs/xfs_dir2_data.c |    1 +
>  libxfs/xfs_dir2_leaf.c |    1 +
>  libxfs/xfs_dir2_node.c |    2 +-
>  4 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libxfs/xfs_da_btree.c b/libxfs/xfs_da_btree.c
> index b731b54..bc73cbc 100644
> --- a/libxfs/xfs_da_btree.c
> +++ b/libxfs/xfs_da_btree.c
> @@ -562,6 +562,7 @@ xfs_da3_root_split(
>  	if (error)
>  		return error;
>  	node = bp->b_addr;
> +	memset(bp->b_addr, 0, bp->b_bcount);
>  	oldroot = blk1->bp->b_addr;
>  	if (oldroot->hdr.info.magic == cpu_to_be16(XFS_DA_NODE_MAGIC) ||
>  	    oldroot->hdr.info.magic == cpu_to_be16(XFS_DA3_NODE_MAGIC)) {
> diff --git a/libxfs/xfs_dir2_data.c b/libxfs/xfs_dir2_data.c
> index dc9df4d..79c2fc2 100644
> --- a/libxfs/xfs_dir2_data.c
> +++ b/libxfs/xfs_dir2_data.c
> @@ -559,6 +559,7 @@ xfs_dir3_data_init(
>  		XFS_DATA_FORK);
>  	if (error)
>  		return error;
> +	memset(bp->b_addr, 0, bp->b_bcount);
>  	bp->b_ops = &xfs_dir3_data_buf_ops;
>  	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DIR_DATA_BUF);
>  
> diff --git a/libxfs/xfs_dir2_leaf.c b/libxfs/xfs_dir2_leaf.c
> index 8e0cbc9..6eb03fc 100644
> --- a/libxfs/xfs_dir2_leaf.c
> +++ b/libxfs/xfs_dir2_leaf.c
> @@ -382,6 +382,7 @@ xfs_dir3_leaf_get_buf(
>  	if (error)
>  		return error;
>  
> +	memset(bp->b_addr, 0, bp->b_bcount);
>  	xfs_dir3_leaf_init(mp, tp, bp, dp->i_ino, magic);
>  	xfs_dir3_leaf_log_header(tp, bp);
>  	if (magic == XFS_DIR2_LEAF1_MAGIC)
> diff --git a/libxfs/xfs_dir2_node.c b/libxfs/xfs_dir2_node.c
> index 3737e4e..3e2f04d 100644
> --- a/libxfs/xfs_dir2_node.c
> +++ b/libxfs/xfs_dir2_node.c
> @@ -247,7 +247,7 @@ xfs_dir3_free_get_buf(
>  	 * Initialize the new block to be empty, and remember
>  	 * its first slot as our empty slot.
>  	 */
> -	memset(bp->b_addr, 0, sizeof(struct xfs_dir3_free_hdr));
> +	memset(bp->b_addr, 0, bp->b_bcount);
>  	memset(&hdr, 0, sizeof(hdr));
>  
>  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

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

[Darrick J. Wong]

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.  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?  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.

Sprinkling memsets around the init code is a fair amount of work, but it solves
the previous problem without the unnecessary memsets.

--D

> 
> Brian
> 
> >  libxfs/xfs_da_btree.c  |    1 +
> >  libxfs/xfs_dir2_data.c |    1 +
> >  libxfs/xfs_dir2_leaf.c |    1 +
> >  libxfs/xfs_dir2_node.c |    2 +-
> >  4 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libxfs/xfs_da_btree.c b/libxfs/xfs_da_btree.c
> > index b731b54..bc73cbc 100644
> > --- a/libxfs/xfs_da_btree.c
> > +++ b/libxfs/xfs_da_btree.c
> > @@ -562,6 +562,7 @@ xfs_da3_root_split(
> >  	if (error)
> >  		return error;
> >  	node = bp->b_addr;
> > +	memset(bp->b_addr, 0, bp->b_bcount);
> >  	oldroot = blk1->bp->b_addr;
> >  	if (oldroot->hdr.info.magic == cpu_to_be16(XFS_DA_NODE_MAGIC) ||
> >  	    oldroot->hdr.info.magic == cpu_to_be16(XFS_DA3_NODE_MAGIC)) {
> > diff --git a/libxfs/xfs_dir2_data.c b/libxfs/xfs_dir2_data.c
> > index dc9df4d..79c2fc2 100644
> > --- a/libxfs/xfs_dir2_data.c
> > +++ b/libxfs/xfs_dir2_data.c
> > @@ -559,6 +559,7 @@ xfs_dir3_data_init(
> >  		XFS_DATA_FORK);
> >  	if (error)
> >  		return error;
> > +	memset(bp->b_addr, 0, bp->b_bcount);
> >  	bp->b_ops = &xfs_dir3_data_buf_ops;
> >  	xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DIR_DATA_BUF);
> >  
> > diff --git a/libxfs/xfs_dir2_leaf.c b/libxfs/xfs_dir2_leaf.c
> > index 8e0cbc9..6eb03fc 100644
> > --- a/libxfs/xfs_dir2_leaf.c
> > +++ b/libxfs/xfs_dir2_leaf.c
> > @@ -382,6 +382,7 @@ xfs_dir3_leaf_get_buf(
> >  	if (error)
> >  		return error;
> >  
> > +	memset(bp->b_addr, 0, bp->b_bcount);
> >  	xfs_dir3_leaf_init(mp, tp, bp, dp->i_ino, magic);
> >  	xfs_dir3_leaf_log_header(tp, bp);
> >  	if (magic == XFS_DIR2_LEAF1_MAGIC)
> > diff --git a/libxfs/xfs_dir2_node.c b/libxfs/xfs_dir2_node.c
> > index 3737e4e..3e2f04d 100644
> > --- a/libxfs/xfs_dir2_node.c
> > +++ b/libxfs/xfs_dir2_node.c
> > @@ -247,7 +247,7 @@ xfs_dir3_free_get_buf(
> >  	 * Initialize the new block to be empty, and remember
> >  	 * its first slot as our empty slot.
> >  	 */
> > -	memset(bp->b_addr, 0, sizeof(struct xfs_dir3_free_hdr));
> > +	memset(bp->b_addr, 0, bp->b_bcount);
> >  	memset(&hdr, 0, sizeof(hdr));
> >  
> >  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

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

[Dave Chinner]

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.

> 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...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Darrick J. Wong]

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

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

[Dave Chinner]

On Thu, Mar 19, 2015 at 04:25:30PM -0700, Darrick J. Wong wrote:
> 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:
> > > 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.

We don't rebuild directories until we've already resolved all the
used/free block references in the filesystem (phase 3/4) and then
rebuilt the free space trees (phase 5). Hence by the end of phase 5
we do not have any doubly referenced blocks, nor do we have any
dangling references in inodes to free space, and we have a fully
correct free space index written to disk.

We then rebuild directories (phase 6) using the corrected free space
btrees if we need to allocate new blocks and hence we can't get
doubly linked blocks in the manner you suggest unless we've broken
repair rather badly...

> OTOH it's probably hard to make that happen.

*nod*

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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