[pseudo][PATCH 1/1] Filter out erroneous POSIX ACLs

[pseudo][PATCH 1/1] Filter out erroneous POSIX ACLs

[anton]

From: Anton Gerasimov <anton@advancedtelematic.com>

The difference between what we see in pseudo and what happens without
pseudo can be seen by typing:

    mkdir setfattr-test
    setfattr -n system.posix_acl_default -v 0x02000000 setfattr-test
    getfattr -n system.posix_acl_default setfattr-test

Under some kernel configurations this difference leads to annoying
errors, e.g. directories copied with 'cp -a' get broken in a fancy way.

Signed-off-by: Anton Gerasimov <anton@advancedtelematic.com>
---
 ports/linux/xattr/pseudo_wrappers.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/ports/linux/xattr/pseudo_wrappers.c b/ports/linux/xattr/pseudo_wrappers.c
index 46bc053..31a6baf 100644
--- a/ports/linux/xattr/pseudo_wrappers.c
+++ b/ports/linux/xattr/pseudo_wrappers.c
@@ -64,7 +64,7 @@ posix_permissions(const acl_header *header, int entries, int *extra, int *mode)
 	if (le32(header->version) != 2) {
 		pseudo_diag("Fatal: ACL support no available for header version %d.\n",
 			le32(header->version));
-		return 1;
+		return -1;
 	}
 	*mode = 0;
 	*extra = 0;
@@ -140,12 +140,27 @@ static int shared_setxattr(const char *path, int fd, const char *name, const voi
 	pseudo_debug(PDBGF_XATTR, "setxattr(%s [fd %d], %s => '%.*s')\n",
 		path ? path : "<no path>", fd, name, (int) size, (char *) value);
 
+	/* Filter out erroneous sizes for POSIX ACL
+	 *  see posix_acl_xattr_count in include/linux/posix_acl_xattr.h of Linux source code */
+	if (!strcmp(name, "system.posix_acl_access") || !strcmp(name, "system.posix_acl_default")) {
+		// ACL is corrupt, issue an error
+		if(size < sizeof(acl_header) || (size - sizeof(acl_header)) % sizeof(acl_entry) != 0) {
+			errno = EINVAL;
+			return -1;
+		}
+
+		// ACL is empty, do nothing
+		if((size - sizeof(acl_header)) / sizeof(acl_entry) == 0) {
+			return 0;
+		}
+	}
 	/* this may be a plain chmod */
 	if (!strcmp(name, "system.posix_acl_access")) {
 		int extra;
 		int mode;
 		int entries = (size - sizeof(acl_header)) / sizeof(acl_entry);
-		if (!posix_permissions(value, entries, &extra, &mode)) {
+		int res = posix_permissions(value, entries, &extra, &mode);
+		if (res == 0) {
 			pseudo_debug(PDBGF_XATTR, "posix_acl_access translated to mode %04o. Remaining attribute(s): %d.\n",
 				mode, extra);
 			buf.st_mode = mode;
@@ -164,8 +179,12 @@ static int shared_setxattr(const char *path, int fd, const char *name, const voi
 			if (!extra) {
 				return 0;
 			}
+		} else if (res == -1) {
+			errno = EOPNOTSUPP;
+			return -1;
 		}
 	}
+
 	if (!strcmp(name, "user.pseudo_data")) {
 		pseudo_debug(PDBGF_XATTR | PDBGF_XATTRDB, "user.pseudo_data xattribute does not get to go in database.\n");
 		return -1;
-- 
2.11.1

[pseudo][PATCH 1/1] Filter out erroneous POSIX ACLs

[anton]

From: Anton Gerasimov <anton@advancedtelematic.com>

The difference between what we see in pseudo and what happens without
pseudo can be seen by typing:

    mkdir setfattr-test
    setfattr -n system.posix_acl_default -v 0x02000000 setfattr-test
    getfattr -n system.posix_acl_default setfattr-test

Under some kernel configurations this difference leads to annoying
errors, e.g. directories copied with 'cp -a' get broken in a fancy way.

Signed-off-by: Anton Gerasimov <anton@advancedtelematic.com>
---
 ports/linux/xattr/pseudo_wrappers.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/ports/linux/xattr/pseudo_wrappers.c b/ports/linux/xattr/pseudo_wrappers.c
index 46bc053..31a6baf 100644
--- a/ports/linux/xattr/pseudo_wrappers.c
+++ b/ports/linux/xattr/pseudo_wrappers.c
@@ -64,7 +64,7 @@ posix_permissions(const acl_header *header, int entries, int *extra, int *mode)
 	if (le32(header->version) != 2) {
 		pseudo_diag("Fatal: ACL support no available for header version %d.\n",
 			le32(header->version));
-		return 1;
+		return -1;
 	}
 	*mode = 0;
 	*extra = 0;
@@ -140,12 +140,27 @@ static int shared_setxattr(const char *path, int fd, const char *name, const voi
 	pseudo_debug(PDBGF_XATTR, "setxattr(%s [fd %d], %s => '%.*s')\n",
 		path ? path : "<no path>", fd, name, (int) size, (char *) value);
 
+	/* Filter out erroneous sizes for POSIX ACL
+	 *  see posix_acl_xattr_count in include/linux/posix_acl_xattr.h of Linux source code */
+	if (!strcmp(name, "system.posix_acl_access") || !strcmp(name, "system.posix_acl_default")) {
+		// ACL is corrupt, issue an error
+		if(size < sizeof(acl_header) || (size - sizeof(acl_header)) % sizeof(acl_entry) != 0) {
+			errno = EINVAL;
+			return -1;
+		}
+
+		// ACL is empty, do nothing
+		if((size - sizeof(acl_header)) / sizeof(acl_entry) == 0) {
+			return 0;
+		}
+	}
 	/* this may be a plain chmod */
 	if (!strcmp(name, "system.posix_acl_access")) {
 		int extra;
 		int mode;
 		int entries = (size - sizeof(acl_header)) / sizeof(acl_entry);
-		if (!posix_permissions(value, entries, &extra, &mode)) {
+		int res = posix_permissions(value, entries, &extra, &mode);
+		if (res == 0) {
 			pseudo_debug(PDBGF_XATTR, "posix_acl_access translated to mode %04o. Remaining attribute(s): %d.\n",
 				mode, extra);
 			buf.st_mode = mode;
@@ -164,8 +179,12 @@ static int shared_setxattr(const char *path, int fd, const char *name, const voi
 			if (!extra) {
 				return 0;
 			}
+		} else if (res == -1) {
+			errno = EOPNOTSUPP;
+			return -1;
 		}
 	}
+
 	if (!strcmp(name, "user.pseudo_data")) {
 		pseudo_debug(PDBGF_XATTR | PDBGF_XATTRDB, "user.pseudo_data xattribute does not get to go in database.\n");
 		return -1;
-- 
2.11.1

Re: [pseudo][PATCH 1/1] Filter out erroneous POSIX ACLs

[Seebs]

On Fri, 24 Feb 2017 15:48:15 +0100
anton@advancedtelematic.com wrote:

> From: Anton Gerasimov <anton@advancedtelematic.com>
> 
> The difference between what we see in pseudo and what happens without
> pseudo can be seen by typing:
> 
>     mkdir setfattr-test
>     setfattr -n system.posix_acl_default -v 0x02000000 setfattr-test
>     getfattr -n system.posix_acl_default setfattr-test
> 
> Under some kernel configurations this difference leads to annoying
> errors, e.g. directories copied with 'cp -a' get broken in a fancy
> way.

Can you expand on this? What is happening to make the "erroneous" ACLs?
What kernel configurations?

-s

Re: [pseudo][PATCH 1/1] Filter out erroneous POSIX ACLs

[Anton Gerasimov]

Hi Peter,

to reproduce the difference, you just need a kernel with POSIX ACL
enabled for your FS (e.g. CONFIG_FS_POSIX_ACL=y;
CONFIG_EXT4_FS_POSIX_ACL=y). The problem is that 'cp -a' when copying a
directory without any ACLs attached will for whatever reason try to
assign an empty posix_acl_default list to the target directory. Linux
kernel will always filter it out, but pseudo just creates empty list.
Later in the build process, when bootable image is created, it will
instantiate a directory with empty posix_acl_default list on the
bootable media. When you will try to perform certain operations on such
directory (e.g. creating another directory inside it), kernel, namely
ext4_get_acl, if I resemble correctly, will return EINVAL error, so the
resulting filesystem will be effectively broken in many ways.

There is an issue [1] in Automotive Grade Linux Jira with an example of
erroneous behaviour. The tricky part of it is that the error itself can
not be reproduced easily: although I have one kernel config of a machine
that makes broken images, I couldn't repeat it on later kernel versions.
But the setfattr/getfattr behaviour difference that causes that error
can be reproduced quite easily on any kernel with ACLs enabled.

[1] (Starting with Jan-Simon Moeller's comment on 26th of October)
https://jira.automotivelinux.org/browse/SPEC-254?focusedCommentId=11313&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-11313

On 02/24/2017 04:05 PM, Seebs wrote:
> On Fri, 24 Feb 2017 15:48:15 +0100
> anton@advancedtelematic.com wrote:
>
>> From: Anton Gerasimov <anton@advancedtelematic.com>
>>
>> The difference between what we see in pseudo and what happens without
>> pseudo can be seen by typing:
>>
>>     mkdir setfattr-test
>>     setfattr -n system.posix_acl_default -v 0x02000000 setfattr-test
>>     getfattr -n system.posix_acl_default setfattr-test
>>
>> Under some kernel configurations this difference leads to annoying
>> errors, e.g. directories copied with 'cp -a' get broken in a fancy
>> way.
> Can you expand on this? What is happening to make the "erroneous" ACLs?
> What kernel configurations?
>
> -s

-- 
Anton Gerasimov, ATS Advanced Telematic Systems GmbH
Kantstrasse 162, 10623 Berlin
Managing Directors: Dirk Pöschl, Armin G. Schmidt
Register Court: HRB 151501 B, Amtsgericht Charlottenburg

Re: [pseudo][PATCH 1/1] Filter out erroneous POSIX ACLs

[Seebs]

On Fri, 24 Feb 2017 16:18:38 +0100
Anton Gerasimov <anton@advancedtelematic.com> wrote:

> FIG_FS_POSIX_ACL=y;
> CONFIG_EXT4_FS_POSIX_ACL=y). The problem is that 'cp -a' when copying
> a directory without any ACLs attached will for whatever reason try to
> assign an empty posix_acl_default list to the target directory.

...

It had totally failed to occur to me to check for *empty* lists. I'll
want to re-read the code just in case, but that makes sense and the
patch looked reasonable.

-s