diff mbox series

[v3,04/28] xfs: Add xfs_verify_pptr

Message ID 20231006184922.252188-5-aalbersh@redhat.com (mailing list archive)
State Superseded
Headers show
Series fs-verity support for XFS | expand

Commit Message

Andrey Albershteyn Oct. 6, 2023, 6:48 p.m. UTC 
From: Allison Henderson <allison.henderson@oracle.com>

Attribute names of parent pointers are not strings.  So we need to modify
attr_namecheck to verify parent pointer records when the XFS_ATTR_PARENT flag is
set.

Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c      | 47 ++++++++++++++++++++++++++++++++---
 fs/xfs/libxfs/xfs_attr.h      |  3 ++-
 fs/xfs/libxfs/xfs_da_format.h |  8 ++++++
 fs/xfs/scrub/attr.c           |  2 +-
 fs/xfs/xfs_attr_item.c        | 11 +++++---
 fs/xfs/xfs_attr_list.c        | 17 +++++++++----
 6 files changed, 74 insertions(+), 14 deletions(-)

Comments

Darrick J. Wong Oct. 11, 2023, 1:01 a.m. UTC | #1 
On Fri, Oct 06, 2023 at 08:48:58PM +0200, Andrey Albershteyn wrote:
> From: Allison Henderson <allison.henderson@oracle.com>
> 
> Attribute names of parent pointers are not strings.  So we need to modify
> attr_namecheck to verify parent pointer records when the XFS_ATTR_PARENT flag is
> set.
> 
> Signed-off-by: Allison Henderson <allison.henderson@oracle.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Hi Andrey,

Could you take a look at the latest revision of the parent pointers
patchset, please?  Your version and mine have drifted very far apart
over the summer...

https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=pptrs-fsck

--D

> ---
>  fs/xfs/libxfs/xfs_attr.c      | 47 ++++++++++++++++++++++++++++++++---
>  fs/xfs/libxfs/xfs_attr.h      |  3 ++-
>  fs/xfs/libxfs/xfs_da_format.h |  8 ++++++
>  fs/xfs/scrub/attr.c           |  2 +-
>  fs/xfs/xfs_attr_item.c        | 11 +++++---
>  fs/xfs/xfs_attr_list.c        | 17 +++++++++----
>  6 files changed, 74 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index 101823772bf9..711022742e34 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1577,9 +1577,33 @@ xfs_attr_node_get(
>  	return error;
>  }
>  
> -/* Returns true if the attribute entry name is valid. */
> -bool
> -xfs_attr_namecheck(
> +/*
> + * Verify parent pointer attribute is valid.
> + * Return true on success or false on failure
> + */
> +STATIC bool
> +xfs_verify_pptr(
> +	struct xfs_mount			*mp,
> +	const struct xfs_parent_name_rec	*rec)
> +{
> +	xfs_ino_t				p_ino;
> +	xfs_dir2_dataptr_t			p_diroffset;
> +
> +	p_ino = be64_to_cpu(rec->p_ino);
> +	p_diroffset = be32_to_cpu(rec->p_diroffset);
> +
> +	if (!xfs_verify_ino(mp, p_ino))
> +		return false;
> +
> +	if (p_diroffset > XFS_DIR2_MAX_DATAPTR)
> +		return false;
> +
> +	return true;
> +}
> +
> +/* Returns true if the string attribute entry name is valid. */
> +static bool
> +xfs_str_attr_namecheck(
>  	const void	*name,
>  	size_t		length)
>  {
> @@ -1594,6 +1618,23 @@ xfs_attr_namecheck(
>  	return !memchr(name, 0, length);
>  }
>  
> +/* Returns true if the attribute entry name is valid. */
> +bool
> +xfs_attr_namecheck(
> +	struct xfs_mount	*mp,
> +	const void		*name,
> +	size_t			length,
> +	int			flags)
> +{
> +	if (flags & XFS_ATTR_PARENT) {
> +		if (length != sizeof(struct xfs_parent_name_rec))
> +			return false;
> +		return xfs_verify_pptr(mp, (struct xfs_parent_name_rec *)name);
> +	}
> +
> +	return xfs_str_attr_namecheck(name, length);
> +}
> +
>  int __init
>  xfs_attr_intent_init_cache(void)
>  {
> diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
> index 3e81f3f48560..b79dae788cfb 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -547,7 +547,8 @@ int xfs_attr_get(struct xfs_da_args *args);
>  int xfs_attr_set(struct xfs_da_args *args);
>  int xfs_attr_set_iter(struct xfs_attr_intent *attr);
>  int xfs_attr_remove_iter(struct xfs_attr_intent *attr);
> -bool xfs_attr_namecheck(const void *name, size_t length);
> +bool xfs_attr_namecheck(struct xfs_mount *mp, const void *name, size_t length,
> +			int flags);
>  int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
>  void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
>  			 unsigned int *total);
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 307c8cdb6f10..6deefe03207f 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -741,6 +741,14 @@ xfs_attr3_leaf_name(xfs_attr_leafblock_t *leafp, int idx)
>  	return &((char *)leafp)[be16_to_cpu(entries[idx].nameidx)];
>  }
>  
> +static inline int
> +xfs_attr3_leaf_flags(xfs_attr_leafblock_t *leafp, int idx)
> +{
> +	struct xfs_attr_leaf_entry *entries = xfs_attr3_leaf_entryp(leafp);
> +
> +	return entries[idx].flags;
> +}
> +
>  static inline xfs_attr_leaf_name_remote_t *
>  xfs_attr3_leaf_name_remote(xfs_attr_leafblock_t *leafp, int idx)
>  {
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index 8bc6aa274fa6..f35144704395 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -195,7 +195,7 @@ xchk_xattr_listent(
>  	}
>  
>  	/* Does this name make sense? */
> -	if (!xfs_attr_namecheck(name, namelen)) {
> +	if (!xfs_attr_namecheck(sx->sc->mp, name, namelen, flags)) {
>  		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
>  		goto fail_xref;
>  	}
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index 97ee9d89b5b8..63393216159f 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -593,7 +593,8 @@ xfs_attri_item_recover(
>  	 */
>  	attrp = &attrip->attri_format;
>  	if (!xfs_attri_validate(mp, attrp) ||
> -	    !xfs_attr_namecheck(nv->name.i_addr, nv->name.i_len))
> +	    !xfs_attr_namecheck(mp, nv->name.i_addr, nv->name.i_len,
> +				attrp->alfi_attr_filter))
>  		return -EFSCORRUPTED;
>  
>  	error = xlog_recover_iget(mp,  attrp->alfi_ino, &ip);
> @@ -805,7 +806,8 @@ xlog_recover_attri_commit_pass2(
>  	}
>  
>  	attr_name = item->ri_buf[i].i_addr;
> -	if (!xfs_attr_namecheck(attr_name, attri_formatp->alfi_name_len)) {
> +	if (!xfs_attr_namecheck(mp, attr_name, attri_formatp->alfi_name_len,
> +				attri_formatp->alfi_attr_filter)) {
>  		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
>  				item->ri_buf[i].i_addr, item->ri_buf[i].i_len);
>  		return -EFSCORRUPTED;
> @@ -823,8 +825,9 @@ xlog_recover_attri_commit_pass2(
>  		}
>  
>  		attr_nname = item->ri_buf[i].i_addr;
> -		if (!xfs_attr_namecheck(attr_nname,
> -				attri_formatp->alfi_nname_len)) {
> +		if (!xfs_attr_namecheck(mp, attr_nname,
> +				attri_formatp->alfi_nname_len,
> +				attri_formatp->alfi_attr_filter)) {
>  			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
>  					item->ri_buf[i].i_addr,
>  					item->ri_buf[i].i_len);
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index 99bbbe1a0e44..a51f7f13a352 100644
> --- a/fs/xfs/xfs_attr_list.c
> +++ b/fs/xfs/xfs_attr_list.c
> @@ -58,9 +58,13 @@ xfs_attr_shortform_list(
>  	struct xfs_attr_sf_sort		*sbuf, *sbp;
>  	struct xfs_attr_shortform	*sf;
>  	struct xfs_attr_sf_entry	*sfe;
> +	struct xfs_mount		*mp;
>  	int				sbsize, nsbuf, count, i;
>  	int				error = 0;
>  
> +	ASSERT(context != NULL);
> +	ASSERT(dp != NULL);
> +	mp = dp->i_mount;
>  	sf = (struct xfs_attr_shortform *)dp->i_af.if_u1.if_data;
>  	ASSERT(sf != NULL);
>  	if (!sf->hdr.count)
> @@ -82,8 +86,9 @@ xfs_attr_shortform_list(
>  	     (dp->i_af.if_bytes + sf->hdr.count * 16) < context->bufsize)) {
>  		for (i = 0, sfe = &sf->list[0]; i < sf->hdr.count; i++) {
>  			if (XFS_IS_CORRUPT(context->dp->i_mount,
> -					   !xfs_attr_namecheck(sfe->nameval,
> -							       sfe->namelen)))
> +					   !xfs_attr_namecheck(mp, sfe->nameval,
> +							       sfe->namelen,
> +							       sfe->flags)))
>  				return -EFSCORRUPTED;
>  			context->put_listent(context,
>  					     sfe->flags,
> @@ -174,8 +179,9 @@ xfs_attr_shortform_list(
>  			cursor->offset = 0;
>  		}
>  		if (XFS_IS_CORRUPT(context->dp->i_mount,
> -				   !xfs_attr_namecheck(sbp->name,
> -						       sbp->namelen))) {
> +				   !xfs_attr_namecheck(mp, sbp->name,
> +						       sbp->namelen,
> +						       sbp->flags))) {
>  			error = -EFSCORRUPTED;
>  			goto out;
>  		}
> @@ -465,7 +471,8 @@ xfs_attr3_leaf_list_int(
>  		}
>  
>  		if (XFS_IS_CORRUPT(context->dp->i_mount,
> -				   !xfs_attr_namecheck(name, namelen)))
> +				   !xfs_attr_namecheck(mp, name, namelen,
> +						       entry->flags)))
>  			return -EFSCORRUPTED;
>  		context->put_listent(context, entry->flags,
>  					      name, namelen, valuelen);
> -- 
> 2.40.1
>
Andrey Albershteyn Oct. 11, 2023, 11:09 a.m. UTC | #2 
On 2023-10-10 18:01:43, Darrick J. Wong wrote:
> Could you take a look at the latest revision of the parent pointers
> patchset, please?  Your version and mine have drifted very far apart
> over the summer...
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=pptrs-fsck
> 

Sure thing, sorry, haven't checked if they changed.
diff mbox series

Patch

diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 101823772bf9..711022742e34 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -1577,9 +1577,33 @@  xfs_attr_node_get(
 	return error;
 }
 
-/* Returns true if the attribute entry name is valid. */
-bool
-xfs_attr_namecheck(
+/*
+ * Verify parent pointer attribute is valid.
+ * Return true on success or false on failure
+ */
+STATIC bool
+xfs_verify_pptr(
+	struct xfs_mount			*mp,
+	const struct xfs_parent_name_rec	*rec)
+{
+	xfs_ino_t				p_ino;
+	xfs_dir2_dataptr_t			p_diroffset;
+
+	p_ino = be64_to_cpu(rec->p_ino);
+	p_diroffset = be32_to_cpu(rec->p_diroffset);
+
+	if (!xfs_verify_ino(mp, p_ino))
+		return false;
+
+	if (p_diroffset > XFS_DIR2_MAX_DATAPTR)
+		return false;
+
+	return true;
+}
+
+/* Returns true if the string attribute entry name is valid. */
+static bool
+xfs_str_attr_namecheck(
 	const void	*name,
 	size_t		length)
 {
@@ -1594,6 +1618,23 @@  xfs_attr_namecheck(
 	return !memchr(name, 0, length);
 }
 
+/* Returns true if the attribute entry name is valid. */
+bool
+xfs_attr_namecheck(
+	struct xfs_mount	*mp,
+	const void		*name,
+	size_t			length,
+	int			flags)
+{
+	if (flags & XFS_ATTR_PARENT) {
+		if (length != sizeof(struct xfs_parent_name_rec))
+			return false;
+		return xfs_verify_pptr(mp, (struct xfs_parent_name_rec *)name);
+	}
+
+	return xfs_str_attr_namecheck(name, length);
+}
+
 int __init
 xfs_attr_intent_init_cache(void)
 {
diff --git a/fs/xfs/libxfs/xfs_attr.h b/fs/xfs/libxfs/xfs_attr.h
index 3e81f3f48560..b79dae788cfb 100644
--- a/fs/xfs/libxfs/xfs_attr.h
+++ b/fs/xfs/libxfs/xfs_attr.h
@@ -547,7 +547,8 @@  int xfs_attr_get(struct xfs_da_args *args);
 int xfs_attr_set(struct xfs_da_args *args);
 int xfs_attr_set_iter(struct xfs_attr_intent *attr);
 int xfs_attr_remove_iter(struct xfs_attr_intent *attr);
-bool xfs_attr_namecheck(const void *name, size_t length);
+bool xfs_attr_namecheck(struct xfs_mount *mp, const void *name, size_t length,
+			int flags);
 int xfs_attr_calc_size(struct xfs_da_args *args, int *local);
 void xfs_init_attr_trans(struct xfs_da_args *args, struct xfs_trans_res *tres,
 			 unsigned int *total);
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 307c8cdb6f10..6deefe03207f 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -741,6 +741,14 @@  xfs_attr3_leaf_name(xfs_attr_leafblock_t *leafp, int idx)
 	return &((char *)leafp)[be16_to_cpu(entries[idx].nameidx)];
 }
 
+static inline int
+xfs_attr3_leaf_flags(xfs_attr_leafblock_t *leafp, int idx)
+{
+	struct xfs_attr_leaf_entry *entries = xfs_attr3_leaf_entryp(leafp);
+
+	return entries[idx].flags;
+}
+
 static inline xfs_attr_leaf_name_remote_t *
 xfs_attr3_leaf_name_remote(xfs_attr_leafblock_t *leafp, int idx)
 {
diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index 8bc6aa274fa6..f35144704395 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -195,7 +195,7 @@  xchk_xattr_listent(
 	}
 
 	/* Does this name make sense? */
-	if (!xfs_attr_namecheck(name, namelen)) {
+	if (!xfs_attr_namecheck(sx->sc->mp, name, namelen, flags)) {
 		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
 		goto fail_xref;
 	}
diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
index 97ee9d89b5b8..63393216159f 100644
--- a/fs/xfs/xfs_attr_item.c
+++ b/fs/xfs/xfs_attr_item.c
@@ -593,7 +593,8 @@  xfs_attri_item_recover(
 	 */
 	attrp = &attrip->attri_format;
 	if (!xfs_attri_validate(mp, attrp) ||
-	    !xfs_attr_namecheck(nv->name.i_addr, nv->name.i_len))
+	    !xfs_attr_namecheck(mp, nv->name.i_addr, nv->name.i_len,
+				attrp->alfi_attr_filter))
 		return -EFSCORRUPTED;
 
 	error = xlog_recover_iget(mp,  attrp->alfi_ino, &ip);
@@ -805,7 +806,8 @@  xlog_recover_attri_commit_pass2(
 	}
 
 	attr_name = item->ri_buf[i].i_addr;
-	if (!xfs_attr_namecheck(attr_name, attri_formatp->alfi_name_len)) {
+	if (!xfs_attr_namecheck(mp, attr_name, attri_formatp->alfi_name_len,
+				attri_formatp->alfi_attr_filter)) {
 		XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
 				item->ri_buf[i].i_addr, item->ri_buf[i].i_len);
 		return -EFSCORRUPTED;
@@ -823,8 +825,9 @@  xlog_recover_attri_commit_pass2(
 		}
 
 		attr_nname = item->ri_buf[i].i_addr;
-		if (!xfs_attr_namecheck(attr_nname,
-				attri_formatp->alfi_nname_len)) {
+		if (!xfs_attr_namecheck(mp, attr_nname,
+				attri_formatp->alfi_nname_len,
+				attri_formatp->alfi_attr_filter)) {
 			XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
 					item->ri_buf[i].i_addr,
 					item->ri_buf[i].i_len);
diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
index 99bbbe1a0e44..a51f7f13a352 100644
--- a/fs/xfs/xfs_attr_list.c
+++ b/fs/xfs/xfs_attr_list.c
@@ -58,9 +58,13 @@  xfs_attr_shortform_list(
 	struct xfs_attr_sf_sort		*sbuf, *sbp;
 	struct xfs_attr_shortform	*sf;
 	struct xfs_attr_sf_entry	*sfe;
+	struct xfs_mount		*mp;
 	int				sbsize, nsbuf, count, i;
 	int				error = 0;
 
+	ASSERT(context != NULL);
+	ASSERT(dp != NULL);
+	mp = dp->i_mount;
 	sf = (struct xfs_attr_shortform *)dp->i_af.if_u1.if_data;
 	ASSERT(sf != NULL);
 	if (!sf->hdr.count)
@@ -82,8 +86,9 @@  xfs_attr_shortform_list(
 	     (dp->i_af.if_bytes + sf->hdr.count * 16) < context->bufsize)) {
 		for (i = 0, sfe = &sf->list[0]; i < sf->hdr.count; i++) {
 			if (XFS_IS_CORRUPT(context->dp->i_mount,
-					   !xfs_attr_namecheck(sfe->nameval,
-							       sfe->namelen)))
+					   !xfs_attr_namecheck(mp, sfe->nameval,
+							       sfe->namelen,
+							       sfe->flags)))
 				return -EFSCORRUPTED;
 			context->put_listent(context,
 					     sfe->flags,
@@ -174,8 +179,9 @@  xfs_attr_shortform_list(
 			cursor->offset = 0;
 		}
 		if (XFS_IS_CORRUPT(context->dp->i_mount,
-				   !xfs_attr_namecheck(sbp->name,
-						       sbp->namelen))) {
+				   !xfs_attr_namecheck(mp, sbp->name,
+						       sbp->namelen,
+						       sbp->flags))) {
 			error = -EFSCORRUPTED;
 			goto out;
 		}
@@ -465,7 +471,8 @@  xfs_attr3_leaf_list_int(
 		}
 
 		if (XFS_IS_CORRUPT(context->dp->i_mount,
-				   !xfs_attr_namecheck(name, namelen)))
+				   !xfs_attr_namecheck(mp, name, namelen,
+						       entry->flags)))
 			return -EFSCORRUPTED;
 		context->put_listent(context, entry->flags,
 					      name, namelen, valuelen);