Re: [PATCH v24 03/11] xfs: Set up infrastructure for log atrribute replay

[Allison Henderson <allison.henderson@xxxxxxxxxx>]

On 8/30/21 5:48 PM, Dave Chinner wrote:
> 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 .....
Ok, will fix

> > +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....
Maybe we can add a call to xfs_attri_validate here?  I think we can just 
modify it to directly check the xfs_attri_log_format.

Thanks!
Allison

> Cheers,
>
> Dave.