On Fri, Oct 06, 2023 at 08:48:58PM +0200, Andrey Albershteyn wrote: > From: Allison Henderson <allison.henderson@xxxxxxxxxx> > > 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> > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx> 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 >
