Skip to contentSkip to conversation

xfs_repair: properly detect reserved attribute names

PATCH In review Eric Sandeen 2 participants 3 messages May 14, 2015

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xfs_repair: properly detect reserved attribute names

This function in xfs_repair tries to make sure that if an attr name reserved for acls exists in the root namespace, then its value is a valid acl.

However, because it only compares up to the length of the reserved name, superstrings may match and cause false positive xfs_repair errors.

Ensure that both the length and the content match before flagging it as an error.

---

repair/attr_repair.c +4 -3
index 27442c4..89a5bbf 100644
@@ -747,9 +747,10 @@ valuecheck(
 	void *valuep;
 	int clearit = 0;
 
-	if ((strncmp(namevalue, SGI_ACL_FILE, SGI_ACL_FILE_SIZE) == 0) ||
-			(strncmp(namevalue, SGI_ACL_DEFAULT,
-				SGI_ACL_DEFAULT_SIZE) == 0)) {
+	if ((namelen == SGI_ACL_FILE_SIZE &&
+	     strncmp(namevalue, SGI_ACL_FILE, SGI_ACL_FILE_SIZE) == 0) ||
+	    (namelen == SGI_ACL_DEFAULT_SIZE &&
+	     strncmp(namevalue, SGI_ACL_DEFAULT, SGI_ACL_DEFAULT_SIZE) == 0)) {
 		if (value == NULL) {
 			valuep = malloc(valuelen);
 			if (!valuep)

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

Reviewed-by on v1 1/1
#1 lore raw
Quoted 8 lines · Eric Sandeen

This function in xfs_repair tries to make sure that if an attr name reserved for acls exists in the root namespace, then its value is a valid acl.

However, because it only compares up to the length of the reserved name, superstrings may match and cause false positive xfs_repair errors.

Quoted 6 lines

Ensure that both the length and the content match before flagging it as an error.

---

Looks good:

So it looks like master doesn't have the sizeof(unsigned char *) problem and the 4.1 update branch does, so we want to make sure that kernel fix makes it into the latter...

Quoted 20 lines
repair/attr_repair.c +4 -3
index 27442c4..89a5bbf 100644
@@ -747,9 +747,10 @@ valuecheck(
 	void *valuep;
 	int clearit = 0;
 
-	if ((strncmp(namevalue, SGI_ACL_FILE, SGI_ACL_FILE_SIZE) == 0) ||
-			(strncmp(namevalue, SGI_ACL_DEFAULT,
-				SGI_ACL_DEFAULT_SIZE) == 0)) {
+	if ((namelen == SGI_ACL_FILE_SIZE &&
+	     strncmp(namevalue, SGI_ACL_FILE, SGI_ACL_FILE_SIZE) == 0) ||
+	    (namelen == SGI_ACL_DEFAULT_SIZE &&
+	     strncmp(namevalue, SGI_ACL_DEFAULT, SGI_ACL_DEFAULT_SIZE) == 0)) {
 		if (value == NULL) {
 			valuep = malloc(valuelen);
 			if (!valuep)
Quoted 4 lines

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

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

#2 lore raw
Quoted 16 lines · Brian Foster
Quoted 8 lines · Eric Sandeen

This function in xfs_repair tries to make sure that if an attr name reserved for acls exists in the root namespace, then its value is a valid acl.

However, because it only compares up to the length of the reserved name, superstrings may match and cause false positive xfs_repair errors.

Quoted 6 lines

Ensure that both the length and the content match before flagging it as an error.

---

Quoted 7 lines

Looks good:

So it looks like master doesn't have the sizeof(unsigned char *) problem and the 4.1 update branch does, so we want to make sure that kernel fix makes it into the latter...

Yep. This fix is needed independently of that, though.

Thanks!
-Eric

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