Re: [PATCH] repair: validate acl count before reading it

[Dave Chinner <david@fromorbit.com>]

On Tue, Nov 15, 2011 at 03:07:15AM -0500, Christoph Hellwig wrote:
> This prevents a segfault on a filesystem so badly corrupted by the RAID
> controller that it could be considered fuzzed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: xfsprogs-dev/repair/attr_repair.c
> ===================================================================
> --- xfsprogs-dev.orig/repair/attr_repair.c	2011-11-14 20:03:27.000000000 +0000
> +++ xfsprogs-dev/repair/attr_repair.c	2011-11-14 20:20:55.000000000 +0000
> @@ -931,8 +931,8 @@ process_longform_attr(
>  }
>  
>  
> -static xfs_acl_t *
> -xfs_acl_from_disk(xfs_acl_disk_t *dacl)
> +static int
> +xfs_acl_from_disk(struct xfs_acl **aclp, struct xfs_acl_disk *dacl)
>  {
>  	int			count;
>  	xfs_acl_t		*acl;
> @@ -940,10 +940,22 @@ xfs_acl_from_disk(xfs_acl_disk_t *dacl)
>  	xfs_acl_entry_disk_t	*dace, *end;
>  
>  	count = be32_to_cpu(dacl->acl_cnt);
> +	if (count > XFS_ACL_MAX_ENTRIES) {
> +		do_warn(_("to larget ACL, size %d"), count);

                           "Too many ACL entries, count %d\n" 

> +		*aclp = NULL;
> +		return EINVAL;
> +	}
> +
> +
>  	end = &dacl->acl_entry[0] + count;
>  	acl = malloc((int)((char *)end - (char *)dacl));
> -	if (!acl)
> -		return NULL;
> +	if (!acl) {
> +		do_warn(_("cannot malloc enough for ACL attribute\n"));
> +		do_warn(_("SKIPPING this ACL\n"));

Should you put that same "Skipping" message for all the error cases?

FWIW, should that status be stored somewhere so that when repair
completes it can emit a warning saying something like:

WARNING: ACLs were not correctly validated. You need to ensure ACLs are
	consistently and appropriately applied to your filesytem.

Regardless, that can be done as a separate patch.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs