On 8/30/21 5:48 PM, Dave Chinner wrote:

On Tue, Aug 24, 2021 at 03:44:26PM -0700, Allison Henderson wrote:

quoted

+/*
+ * 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

quoted

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