On Sat, Jun 11, 2022 at 02:41:51AM -0700, Allison Henderson wrote:
> 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@xxxxxxxxxx>
Looks good,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> ---
> fs/xfs/libxfs/xfs_attr.c | 43 +++++++++++++++++++++++++++++++++++++---
> fs/xfs/libxfs/xfs_attr.h | 3 ++-
> fs/xfs/scrub/attr.c | 2 +-
> fs/xfs/xfs_attr_item.c | 6 ++++--
> fs/xfs/xfs_attr_list.c | 17 +++++++++++-----
> 5 files changed, 59 insertions(+), 12 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> index ee5dfebcf163..30c8d9e9c2f1 100644
> --- a/fs/xfs/libxfs/xfs_attr.c
> +++ b/fs/xfs/libxfs/xfs_attr.c
> @@ -1606,9 +1606,29 @@ 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, struct xfs_parent_name_rec *rec)
> +{
> + xfs_ino_t p_ino = (xfs_ino_t)be64_to_cpu(rec->p_ino);
> + xfs_dir2_dataptr_t p_diroffset =
> + (xfs_dir2_dataptr_t)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)
> {
> @@ -1623,6 +1643,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 7600eac74db7..a87bc503976b 100644
> --- a/fs/xfs/libxfs/xfs_attr.h
> +++ b/fs/xfs/libxfs/xfs_attr.h
> @@ -562,7 +562,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/scrub/attr.c b/fs/xfs/scrub/attr.c
> index b6f0c9f3f124..d3e75c077fab 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -128,7 +128,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);
> return;
> }
> diff --git a/fs/xfs/xfs_attr_item.c b/fs/xfs/xfs_attr_item.c
> index f524dbbb42d3..60fee5814655 100644
> --- a/fs/xfs/xfs_attr_item.c
> +++ b/fs/xfs/xfs_attr_item.c
> @@ -587,7 +587,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);
> @@ -742,7 +743,8 @@ xlog_recover_attri_commit_pass2(
> return -EFSCORRUPTED;
> }
>
> - 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_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> return -EFSCORRUPTED;
> }
> diff --git a/fs/xfs/xfs_attr_list.c b/fs/xfs/xfs_attr_list.c
> index 90a14e85e76d..3bac0647a927 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;
> ASSERT(dp->i_afp != NULL);
> sf = (struct xfs_attr_shortform *)dp->i_afp->if_u1.if_data;
> ASSERT(sf != NULL);
> @@ -83,8 +87,9 @@ xfs_attr_shortform_list(
> (dp->i_afp->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,
> @@ -175,8 +180,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;
> }
> @@ -466,7 +472,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.25.1
>
