[1/4] xfs: check attribute leaf block structure
diff mbox series

Message ID 157198049357.2873445.8604948103647704008.stgit@magnolia
State Superseded
Headers show
Series
  • xfs: more metadata verifier tightening
Related show

Commit Message

Darrick J. Wong Oct. 25, 2019, 5:14 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Add missing structure checks in the attribute leaf verifier.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c |   63 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 61 insertions(+), 2 deletions(-)

Comments

Brian Foster Oct. 28, 2019, 6:18 p.m. UTC | #1
On Thu, Oct 24, 2019 at 10:14:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add missing structure checks in the attribute leaf verifier.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c |   63 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index ec7921e07f69..8dea3a273029 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -232,6 +232,57 @@ xfs_attr3_leaf_hdr_to_disk(
>  	}
>  }
>  
> +static xfs_failaddr_t
> +xfs_attr3_leaf_verify_entry(
> +	struct xfs_mount			*mp,
> +	char					*buf_end,
> +	struct xfs_attr_leafblock		*leaf,
> +	struct xfs_attr3_icleaf_hdr		*leafhdr,
> +	struct xfs_attr_leaf_entry		*ent,
> +	int					idx,
> +	__u32					*last_hashval)
> +{
> +	struct xfs_attr_leaf_name_local		*lentry;
> +	struct xfs_attr_leaf_name_remote	*rentry;
> +	char					*name_end;
> +	unsigned int				nameidx;
> +	unsigned int				namesize;
> +	__u32					hashval;
> +
> +	/* hash order check */
> +	hashval = be32_to_cpu(ent->hashval);
> +	if (hashval < *last_hashval)
> +		return __this_address;
> +	*last_hashval = hashval;
> +
> +	nameidx = be16_to_cpu(ent->nameidx);
> +	if (nameidx < leafhdr->firstused || nameidx >= mp->m_attr_geo->blksize)
> +		return __this_address;
> +
> +	/* Check the name information. */
> +	if (ent->flags & XFS_ATTR_LOCAL) {
> +		lentry = xfs_attr3_leaf_name_local(leaf, idx);
> +		namesize = xfs_attr_leaf_entsize_local(lentry->namelen,
> +				be16_to_cpu(lentry->valuelen));
> +		name_end = (char *)lentry + namesize;
> +		if (lentry->namelen == 0)
> +			return __this_address;

I think this reads a little better if we check the lentry value before
we use it (same deal with rentry in the branch below).

Also, why the == 0 checks specifically? Or IOW, might there also be a
sane max value to check some of these fields against?

> +	} else {
> +		rentry = xfs_attr3_leaf_name_remote(leaf, idx);
> +		namesize = xfs_attr_leaf_entsize_remote(rentry->namelen);
> +		name_end = (char *)rentry + namesize;
> +		if (rentry->namelen == 0)
> +			return __this_address;
> +		if (rentry->valueblk == 0)
> +			return __this_address;

Hmm.. ISTR that it's currently possible to have ->valueblk == 0 on an
incomplete remote attr after a crash. That's not ideal and hopefully
fixed up after the xattr intent stuff lands, but in the meantime I
thought we had code sprinkled around somewhere to fix that up after the
fact. Would this turn that scenario into a metadata I/O error?

Brian

> +	}
> +
> +	if (name_end > buf_end)
> +		return __this_address;
> +
> +	return NULL;
> +}
> +
>  static xfs_failaddr_t
>  xfs_attr3_leaf_verify(
>  	struct xfs_buf			*bp)
> @@ -240,7 +291,10 @@ xfs_attr3_leaf_verify(
>  	struct xfs_mount		*mp = bp->b_mount;
>  	struct xfs_attr_leafblock	*leaf = bp->b_addr;
>  	struct xfs_attr_leaf_entry	*entries;
> +	struct xfs_attr_leaf_entry	*ent;
> +	char				*buf_end;
>  	uint32_t			end;	/* must be 32bit - see below */
> +	__u32				last_hashval = 0;
>  	int				i;
>  	xfs_failaddr_t			fa;
>  
> @@ -273,8 +327,13 @@ xfs_attr3_leaf_verify(
>  	    (char *)bp->b_addr + ichdr.firstused)
>  		return __this_address;
>  
> -	/* XXX: need to range check rest of attr header values */
> -	/* XXX: hash order check? */
> +	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
> +	for (i = 0, ent = entries; i < ichdr.count; ent++, i++) {
> +		fa = xfs_attr3_leaf_verify_entry(mp, buf_end, leaf, &ichdr,
> +				ent, i, &last_hashval);
> +		if (fa)
> +			return fa;
> +	}
>  
>  	/*
>  	 * Quickly check the freemap information.  Attribute data has to be
>
Darrick J. Wong Oct. 28, 2019, 6:27 p.m. UTC | #2
On Mon, Oct 28, 2019 at 02:18:13PM -0400, Brian Foster wrote:
> On Thu, Oct 24, 2019 at 10:14:53PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add missing structure checks in the attribute leaf verifier.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr_leaf.c |   63 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 61 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index ec7921e07f69..8dea3a273029 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -232,6 +232,57 @@ xfs_attr3_leaf_hdr_to_disk(
> >  	}
> >  }
> >  
> > +static xfs_failaddr_t
> > +xfs_attr3_leaf_verify_entry(
> > +	struct xfs_mount			*mp,
> > +	char					*buf_end,
> > +	struct xfs_attr_leafblock		*leaf,
> > +	struct xfs_attr3_icleaf_hdr		*leafhdr,
> > +	struct xfs_attr_leaf_entry		*ent,
> > +	int					idx,
> > +	__u32					*last_hashval)
> > +{
> > +	struct xfs_attr_leaf_name_local		*lentry;
> > +	struct xfs_attr_leaf_name_remote	*rentry;
> > +	char					*name_end;
> > +	unsigned int				nameidx;
> > +	unsigned int				namesize;
> > +	__u32					hashval;
> > +
> > +	/* hash order check */
> > +	hashval = be32_to_cpu(ent->hashval);
> > +	if (hashval < *last_hashval)
> > +		return __this_address;
> > +	*last_hashval = hashval;
> > +
> > +	nameidx = be16_to_cpu(ent->nameidx);
> > +	if (nameidx < leafhdr->firstused || nameidx >= mp->m_attr_geo->blksize)
> > +		return __this_address;
> > +
> > +	/* Check the name information. */
> > +	if (ent->flags & XFS_ATTR_LOCAL) {
> > +		lentry = xfs_attr3_leaf_name_local(leaf, idx);
> > +		namesize = xfs_attr_leaf_entsize_local(lentry->namelen,
> > +				be16_to_cpu(lentry->valuelen));
> > +		name_end = (char *)lentry + namesize;
> > +		if (lentry->namelen == 0)
> > +			return __this_address;
> 
> I think this reads a little better if we check the lentry value before
> we use it (same deal with rentry in the branch below).
> 
> Also, why the == 0 checks specifically? Or IOW, might there also be a
> sane max value to check some of these fields against?

Attributes have a maximum name length of 255 characters, and the ondisk
namelen field is u8, so it's never possible to exceed the maximum.

> > +	} else {
> > +		rentry = xfs_attr3_leaf_name_remote(leaf, idx);
> > +		namesize = xfs_attr_leaf_entsize_remote(rentry->namelen);
> > +		name_end = (char *)rentry + namesize;
> > +		if (rentry->namelen == 0)
> > +			return __this_address;
> > +		if (rentry->valueblk == 0)
> > +			return __this_address;
> 
> Hmm.. ISTR that it's currently possible to have ->valueblk == 0 on an
> incomplete remote attr after a crash. That's not ideal and hopefully
> fixed up after the xattr intent stuff lands, but in the meantime I
> thought we had code sprinkled around somewhere to fix that up after the
> fact. Would this turn that scenario into a metadata I/O error?

<urk> Yes, it would.  I'll fix that.

--D

> 
> Brian
> 
> > +	}
> > +
> > +	if (name_end > buf_end)
> > +		return __this_address;
> > +
> > +	return NULL;
> > +}
> > +
> >  static xfs_failaddr_t
> >  xfs_attr3_leaf_verify(
> >  	struct xfs_buf			*bp)
> > @@ -240,7 +291,10 @@ xfs_attr3_leaf_verify(
> >  	struct xfs_mount		*mp = bp->b_mount;
> >  	struct xfs_attr_leafblock	*leaf = bp->b_addr;
> >  	struct xfs_attr_leaf_entry	*entries;
> > +	struct xfs_attr_leaf_entry	*ent;
> > +	char				*buf_end;
> >  	uint32_t			end;	/* must be 32bit - see below */
> > +	__u32				last_hashval = 0;
> >  	int				i;
> >  	xfs_failaddr_t			fa;
> >  
> > @@ -273,8 +327,13 @@ xfs_attr3_leaf_verify(
> >  	    (char *)bp->b_addr + ichdr.firstused)
> >  		return __this_address;
> >  
> > -	/* XXX: need to range check rest of attr header values */
> > -	/* XXX: hash order check? */
> > +	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
> > +	for (i = 0, ent = entries; i < ichdr.count; ent++, i++) {
> > +		fa = xfs_attr3_leaf_verify_entry(mp, buf_end, leaf, &ichdr,
> > +				ent, i, &last_hashval);
> > +		if (fa)
> > +			return fa;
> > +	}
> >  
> >  	/*
> >  	 * Quickly check the freemap information.  Attribute data has to be
> > 
>

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index ec7921e07f69..8dea3a273029 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -232,6 +232,57 @@  xfs_attr3_leaf_hdr_to_disk(
 	}
 }
 
+static xfs_failaddr_t
+xfs_attr3_leaf_verify_entry(
+	struct xfs_mount			*mp,
+	char					*buf_end,
+	struct xfs_attr_leafblock		*leaf,
+	struct xfs_attr3_icleaf_hdr		*leafhdr,
+	struct xfs_attr_leaf_entry		*ent,
+	int					idx,
+	__u32					*last_hashval)
+{
+	struct xfs_attr_leaf_name_local		*lentry;
+	struct xfs_attr_leaf_name_remote	*rentry;
+	char					*name_end;
+	unsigned int				nameidx;
+	unsigned int				namesize;
+	__u32					hashval;
+
+	/* hash order check */
+	hashval = be32_to_cpu(ent->hashval);
+	if (hashval < *last_hashval)
+		return __this_address;
+	*last_hashval = hashval;
+
+	nameidx = be16_to_cpu(ent->nameidx);
+	if (nameidx < leafhdr->firstused || nameidx >= mp->m_attr_geo->blksize)
+		return __this_address;
+
+	/* Check the name information. */
+	if (ent->flags & XFS_ATTR_LOCAL) {
+		lentry = xfs_attr3_leaf_name_local(leaf, idx);
+		namesize = xfs_attr_leaf_entsize_local(lentry->namelen,
+				be16_to_cpu(lentry->valuelen));
+		name_end = (char *)lentry + namesize;
+		if (lentry->namelen == 0)
+			return __this_address;
+	} else {
+		rentry = xfs_attr3_leaf_name_remote(leaf, idx);
+		namesize = xfs_attr_leaf_entsize_remote(rentry->namelen);
+		name_end = (char *)rentry + namesize;
+		if (rentry->namelen == 0)
+			return __this_address;
+		if (rentry->valueblk == 0)
+			return __this_address;
+	}
+
+	if (name_end > buf_end)
+		return __this_address;
+
+	return NULL;
+}
+
 static xfs_failaddr_t
 xfs_attr3_leaf_verify(
 	struct xfs_buf			*bp)
@@ -240,7 +291,10 @@  xfs_attr3_leaf_verify(
 	struct xfs_mount		*mp = bp->b_mount;
 	struct xfs_attr_leafblock	*leaf = bp->b_addr;
 	struct xfs_attr_leaf_entry	*entries;
+	struct xfs_attr_leaf_entry	*ent;
+	char				*buf_end;
 	uint32_t			end;	/* must be 32bit - see below */
+	__u32				last_hashval = 0;
 	int				i;
 	xfs_failaddr_t			fa;
 
@@ -273,8 +327,13 @@  xfs_attr3_leaf_verify(
 	    (char *)bp->b_addr + ichdr.firstused)
 		return __this_address;
 
-	/* XXX: need to range check rest of attr header values */
-	/* XXX: hash order check? */
+	buf_end = (char *)bp->b_addr + mp->m_attr_geo->blksize;
+	for (i = 0, ent = entries; i < ichdr.count; ent++, i++) {
+		fa = xfs_attr3_leaf_verify_entry(mp, buf_end, leaf, &ichdr,
+				ent, i, &last_hashval);
+		if (fa)
+			return fa;
+	}
 
 	/*
 	 * Quickly check the freemap information.  Attribute data has to be