On Tue, Aug 24, 2021 at 03:44:26PM -0700, Allison Henderson wrote:
> +/*
> + * Allocate and initialize an attri item. Caller may allocate an additional
> + * trailing buffer of the specified size
> + */
> +STATIC struct xfs_attri_log_item *
> +xfs_attri_init(
> + struct xfs_mount *mp,
> + int buffer_size)
> +
> +{
> + struct xfs_attri_log_item *attrip;
> + uint size;
> +
> + size = sizeof(struct xfs_attri_log_item) + buffer_size;
> + attrip = kvmalloc(size, KM_ZERO);
> + if (attrip == NULL)
> + return NULL;
kvmalloc() takes GFP flags. I think you want GFP_KERNEL | __GFP_ZERO
here.
Also, buffer size is taken directly from on-disk without bounds/length
validation, meaning this could end up being an attacker controlled
memory allocation, so .....
> +STATIC int
> +xlog_recover_attri_commit_pass2(
> + struct xlog *log,
> + struct list_head *buffer_list,
> + struct xlog_recover_item *item,
> + xfs_lsn_t lsn)
> +{
> + int error;
> + struct xfs_mount *mp = log->l_mp;
> + struct xfs_attri_log_item *attrip;
> + struct xfs_attri_log_format *attri_formatp;
> + char *name = NULL;
> + char *value = NULL;
> + int region = 0;
> + int buffer_size;
> +
> + attri_formatp = item->ri_buf[region].i_addr;
> +
> + /* Validate xfs_attri_log_format */
> + if (attri_formatp->__pad != 0 || attri_formatp->alfi_name_len == 0 ||
> + (attri_formatp->alfi_op_flags == XFS_ATTR_OP_FLAGS_REMOVE &&
> + attri_formatp->alfi_value_len != 0)) {
> + XFS_ERROR_REPORT(__func__, XFS_ERRLEVEL_LOW, mp);
> + return -EFSCORRUPTED;
> + }
> +
> + buffer_size = attri_formatp->alfi_name_len +
> + attri_formatp->alfi_value_len;
> +
> + attrip = xfs_attri_init(mp, buffer_size);
> + if (attrip == NULL)
> + return -ENOMEM;
There needs to be a lot better validation of the attribute
name/value lengths here. Also, memory allocation failure here will
abort recovery, so it might be worth adding a comment here....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
