[PATCH] repair: validate acl count before reading it

[PATCH] repair: validate acl count before reading it

[Christoph Hellwig]

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);
+		*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"));
+		*aclp = NULL;
+		return ENOMEM;
+	}
+
 	acl->acl_cnt = count;
 	ace = &acl->acl_entry[0];
 	for (dace = &dacl->acl_entry[0]; dace < end; ace++, dace++) {
@@ -951,7 +963,9 @@ xfs_acl_from_disk(xfs_acl_disk_t *dacl)
 		ace->ae_id = be32_to_cpu(dace->ae_id);
 		ace->ae_perm = be16_to_cpu(dace->ae_perm);
 	}
-	return acl;
+
+	*aclp = acl;
+	return 0;
 }
 
 /*
@@ -1004,15 +1018,14 @@ xfs_acl_valid(xfs_acl_disk_t *daclp)
 	if (daclp == NULL)
 		goto acl_invalid;
 
-	aclp = xfs_acl_from_disk(daclp);
-	if (aclp == NULL) {
-		do_warn(_("cannot malloc enough for ACL attribute\n"));
-		do_warn(_("SKIPPING this ACL\n"));
+	switch (xfs_acl_from_disk(&aclp, daclp)) {
+	case ENOMEM:
 		return 0;
-	}
-
-	if (aclp->acl_cnt > XFS_ACL_MAX_ENTRIES)
+	case EINVAL:
 		goto acl_invalid;
+	default:
+		break;
+	}
 
 	for (i = 0; i < aclp->acl_cnt; i++) {
 		entry = &aclp->acl_entry[i];

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

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

[Dave Chinner]

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

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

[Christoph Hellwig]

On Wed, Nov 16, 2011 at 11:23:23AM +1100, Dave Chinner wrote:
> >  	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" 

Ok.

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

For the ENOMEM case we indeed skip the ACL.  For other errors we will
just remove this attribute.  Given how enomem really should not just
happen for that small ACL I wonder how special casing it makes any
sense - but that code has been there for a while.

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

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

[Mark Tinguely]

On 01/-10/63 13:59, 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
> ===================================================================
...

Looks good.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

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