On Wed, Jul 19, 2017 at 09:03:18AM -0700, Darrick J. Wong wrote:
> Create a centralized function to verify local format inode forks, then
> migrate the existing short format directory verifier to use this
> framework and add verifiers for the other two things we support (attrs
> and symlinks).
>
> Obviously this will get broken up into smaller pieces (one patch to
> refactor/move the existing verifier calls, another one to add the two
> new verifiers), but what do people think?
>
> Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_attr_leaf.c | 70 ++++++++++++++++++++++++++++++++++++
> fs/xfs/libxfs/xfs_attr_leaf.h | 1 +
> fs/xfs/libxfs/xfs_inode_fork.c | 10 -----
> fs/xfs/libxfs/xfs_shared.h | 1 +
> fs/xfs/libxfs/xfs_symlink_remote.c | 29 +++++++++++++++
> fs/xfs/xfs_icache.c | 46 ++++++++++++++++++++++++
> fs/xfs/xfs_icache.h | 1 +
> fs/xfs/xfs_inode.c | 6 +--
> 8 files changed, 150 insertions(+), 14 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index c6c15e5..43b881e 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -871,6 +871,76 @@ xfs_attr_shortform_allfit(
> return xfs_attr_shortform_bytesfit(dp, bytes);
> }
>
> +/* Verify the consistency of an inline attribute fork. */
> +int
> +xfs_attr_shortform_verify(
> + struct xfs_inode *ip)
> +{
> + struct xfs_attr_shortform *sfp;
> + struct xfs_attr_sf_entry *sfep;
> + struct xfs_attr_sf_entry *next_sfep;
> + char *endp;
> + struct xfs_ifork *ifp;
> + int i;
> + int size;
> +
> + ASSERT(ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL);
> + ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> + sfp = (struct xfs_attr_shortform *)ifp->if_u1.if_data;
> + size = ifp->if_bytes;
> +
> + /*
> + * Give up if the attribute is way too short.
> + */
> + if (size < sizeof (struct xfs_attr_sf_hdr))
> + return -EFSCORRUPTED;
This seems to already be checked in xfs_iformat_fork() for
attr forks. There are also inconsistent checks between data/attr
forks whether size is valid prior formatting the fork, so there's
more cleanup needed there, and if we are checking attr/dir validity
in these functions, we shouldn't be peeking inside the fork
in xfs_iformat_fork().
> + endp = (char *)sfp + size;
> +
> + /* Check all reported entries */
> + sfep = &sfp->list[0];
> + for (i = 0; i < sfp->hdr.count; i++) {
> + /*
> + * struct xfs_attr_sf_entry has a variable length.
> + * Check the fixed-offset parts of the structure are
> + * within the data buffer.
> + */
> + if (((char *)sfep + sizeof(*sfep)) >= endp)
> + return -EFSCORRUPTED;
> +
> + /* Don't allow names with known bad length. */
> + if (sfep->namelen == 0)
> + return -EFSCORRUPTED;
> +
> + /*
> + * Check that the variable-length part of the structure is
> + * within the data buffer. The next entry starts after the
> + * name component, so nextentry is an acceptable test.
> + */
> + next_sfep = XFS_ATTR_SF_NEXTENTRY(sfep);
> + if (endp < (char *)next_sfep)
> + return -EFSCORRUPTED;
Can we make the "past endp" checks consistent in variable order and
operators? i.e
if ((char *)next_sfep >= endp)
[snip]
> diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
> index c484877..96e2957 100644
> --- a/fs/xfs/libxfs/xfs_symlink_remote.c
> +++ b/fs/xfs/libxfs/xfs_symlink_remote.c
> @@ -207,3 +207,32 @@ xfs_symlink_local_to_remote(
> xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsymlink_hdr) +
> ifp->if_bytes - 1);
> }
> +
> +/* Verify the consistency of an inline symlink. */
> +int
> +xfs_symlink_sf_verify(
> + struct xfs_inode *ip)
> +{
> + char *sfp;
> + char *endp;
> + struct xfs_ifork *ifp;
> + int size;
> +
> + ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> + ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> + sfp = (char *)ifp->if_u1.if_data;
> + size = ifp->if_bytes;
> + endp = sfp + size;
> +
> + /* No negative sizes or overly long symlink targets. */
> + if (size < 0 || size > XFS_SYMLINK_MAXLEN)
> + return -EFSCORRUPTED;
> +
> + /* No NULLs in the target either. */
> + for (; sfp < endp; sfp++) {
> + if (*sfp == 0)
> + return -EFSCORRUPTED;
> + }
Hmmm - do we need to check that the symlink has been correctly null
terminated in memory (i.e. that *endp == 0) because the VFS requires
this and we add the zero termination before checking fork contents
validity?
> +/* Check inline fork contents. */
> +int
> +xfs_iget_verify_forks(
*_verify_inline_content()?
> + struct xfs_inode *ip)
> +{
> + int error;
Too much indenting :P
> +
> + /* Check the attribute fork if there is one. */
> + if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
> + ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
If there is no attr fork, then ip->i_d.di_aformat should be set to
XFS_DINODE_FMT_EXTENTS. Hence we can just do the same check as
for the data fork....
OTOH, having a value of XFS_DINODE_FMT_LOCAL in di_aformat without
an attr fork indicates corruption, so perhaps we need to catch that
here as it's not checked in xfs_ifork_format() or xfs_iflush_int().
Indeed, there are partial attr/data fork format/size checks in
xfs_ifork_format() and xfs_iflush_int(), but we don't do
comprehensive checks in either place. Maybe they should all be moved
inside this function and expanded to check that all the fork format
information is valid?
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
