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

[Allison Henderson <allison.henderson@oracle.com>]

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.
>