Skip to contentSkip to conversation

repair: validate acl count before reading it

PATCH In review Christoph Hellwig 3 participants 4 messages Nov 15, 2011 – Jan 27, 2012

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 repair: validate acl count before reading it

This prevents a segfault on a filesystem so badly corrupted by the RAID controller that it could be considered fuzzed.

repair/attr_repair.c +25 -12
Index: xfsprogs-dev/repair/attr_repair.c
===================================================================
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v1 1/1
#1 lore raw
Quoted 21 lines · Christoph Hellwig

This prevents a segfault on a filesystem so badly corrupted by the RAID controller that it could be considered fuzzed.

Index: xfsprogs-dev/repair/attr_repair.c
===================================================================

repair/attr_repair.c +2 -2
@@ -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;

repair/attr_repair.c xfs_acl_from_disk(xfs_acl_disk_t *dacl) · line 944

Quoted 5 lines
repair/attr_repair.c +2 -0
@@ -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"

repair/attr_repair.c xfs_acl_from_disk(xfs_acl_disk_t *dacl) · line 954

Quoted 12 lines
repair/attr_repair.c +8 -2
+		*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.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#2 lore raw
Quoted 5 lines · Dave Chinner

repair/attr_repair.c xfs_acl_from_disk(xfs_acl_disk_t *dacl) · line 944

Quoted 3 lines
repair/attr_repair.c +2 -0
 	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.

Quoted 6 lines
Quoted 6 lines
repair/attr_repair.c +5 -0
+		*aclp = NULL;
+		return EINVAL;
+	}
+
+
 	end = &dacl->acl_entry[0] + count;
Quoted 8 lines

repair/attr_repair.c xfs_acl_from_disk(xfs_acl_disk_t *dacl) · line 954

Quoted 6 lines
repair/attr_repair.c +3 -2
 	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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Reviewed-by on v1 1/1
#3 lore raw
In reply to Christoph Hellwig
Quoted 7 lines · Christoph Hellwig

This prevents a segfault on a filesystem so badly corrupted by the RAID controller that it could be considered fuzzed.

Index: xfsprogs-dev/repair/attr_repair.c
===================================================================

...

Looks good.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs