Document new xattrperm flag

Document new xattrperm flag

[Marko Petrović]

Hello,

I am Marko Petrović. I have been using User-Mode Linux (UML) for some time
and I have noticed that in the documentation it is said that UML can boot
from hostfs however, hostfs exposes file permissions of the host to the
UML, and changing these permissions requires that the kernel has necessary
privileges on the host.
In addition to that, all files are created with the ownership of the
kernel's user and group since the kernel is performing file creation.

This creates obvious problems when a multiuser system is running inside
UML since applications cannot create files that they own and the UML
kernel forbids further access to these files. This in particular can
present problems when booting from hostfs that appears to otherwise be
supported.

One solution would be for the kernel to run with the necessary privileges
to alter file permissions and yet still access them in order to service
syscalls to UML processes and another (in my humble opinion, preferable)
solution would be to store permissions used by the UML kernel separately
from host's permissions so that the kernel can run with standard
privileges.

In hope that it will be useful, I have written a patch that adds a boot
option for hostfs for enabling the usage of extended attributes for
storing these permissions. Extended attributes seemed like the most
reasonable choice for this purpose and most Linux filesystems support
them.

I have also added a try for doing regular chown(2) on file
creation when extended attributes are disabled. If the kernel isn't
running as root, it will fall back to the old behavior.

In another patch, I provide documentation update for explaining
the usage of the new flag when booting from hostfs. I have also changed
the "find" command that was used there so that it now skips symlinks
since some symlinks point to absolute paths and that was changing
permissions on the host in unintended ways.

I am looking forward to your feedback on this work.

P.S. I apologize if there are any grammar errors in the mail as English is
not my first language. As this is my first patch, I also apologize if I
have missed any part of the patch submission procedure. For future patches
I will correct all encountered mistakes.

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH 1/2] Document new xattrperm flag

[Marko Petrović]

Signed-off-by: Marko Petrović <petrovicmarko2006@gmail.com>
---
 ...to_v2.rst => user_mode_linux_howto_v3.rst} | 20 +++++++++----------
 1 file changed, 9 insertions(+), 11 deletions(-)
 rename Documentation/virt/uml/{user_mode_linux_howto_v2.rst => user_mode_linux_howto_v3.rst} (99%)

diff --git a/Documentation/virt/uml/user_mode_linux_howto_v2.rst b/Documentation/virt/uml/user_mode_linux_howto_v3.rst
similarity index 99%
rename from Documentation/virt/uml/user_mode_linux_howto_v2.rst
rename to Documentation/virt/uml/user_mode_linux_howto_v3.rst
index af2a97429692..709eef10dd7d 100644
--- a/Documentation/virt/uml/user_mode_linux_howto_v2.rst
+++ b/Documentation/virt/uml/user_mode_linux_howto_v3.rst
@@ -1007,23 +1007,21 @@ an existing root_fs file::
 
    #  mount root_fs uml_root_dir -o loop
 
-
-You need to change the filesystem type of ``/`` in ``etc/fstab`` to be
-'hostfs', so that line looks like this::
-
-   /dev/ubd/0       /        hostfs      defaults          1   1
-
 Then you need to chown to yourself all the files in that directory
-that are owned by root.  This worked for me::
+that are owned by root so that the kernel can access them.
+This worked for me::
 
-   #  find . -uid 0 -exec chown jdike {} \;
+   #  find uml_root_dir -uid 0 -not -type l -exec chown jdike {} \;
 
 Next, make sure that your UML kernel has hostfs compiled in, not as a
-module.  Then run UML with the boot device pointing at that directory::
+module.  Then run UML with the appropriate kernel command line
+parameters::
 
-   ubd0=/path/to/uml/root/directory
+   rootfstype=hostfs rw hostfs=uml_root_dir,xattrperm
 
-UML should then boot as it does normally.
+You should have extended attributes supported and enabled on
+your host filesystem since UML uses them to store correct file
+permissions.
 
 Hostfs Caveats
 --------------
-- 
2.39.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH 1/2] Document new xattrperm flag

[Johannes Berg]

Hi,

Nice. I think you can squash this into a single patch eventually. Couple
of comments below:

On Fri, 2023-04-14 at 00:30 +0200, Marko Petrović wrote:
> Signed-off-by: Marko Petrović <petrovicmarko2006@gmail.com>
> ---
>  ...to_v2.rst => user_mode_linux_howto_v3.rst} | 20 +++++++++----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
>  rename Documentation/virt/uml/{user_mode_linux_howto_v2.rst => user_mode_linux_howto_v3.rst} (99%)

I don't think you should rename this, it's not a document version, it's
more of a historic artifact that it's called v2 now. We should probably
remove that anyway.

> +++ b/Documentation/virt/uml/user_mode_linux_howto_v3.rst
> @@ -1007,23 +1007,21 @@ an existing root_fs file::
>  
>     #  mount root_fs uml_root_dir -o loop
>  
> -
> -You need to change the filesystem type of ``/`` in ``etc/fstab`` to be
> -'hostfs', so that line looks like this::
> -
> -   /dev/ubd/0       /        hostfs      defaults          1   1
> -

I was going to ask why you removed this, but yeah, cleaning this up to
not use ubd0 for hostfs is probably a good idea. At least documentation
(recommendation) wise ...

>  Then you need to chown to yourself all the files in that directory
> -that are owned by root.  This worked for me::
> +that are owned by root so that the kernel can access them.
> +This worked for me::
>  
> -   #  find . -uid 0 -exec chown jdike {} \;
> +   #  find uml_root_dir -uid 0 -not -type l -exec chown jdike {} \;
>  
>  Next, make sure that your UML kernel has hostfs compiled in, not as a
> -module.  Then run UML with the boot device pointing at that directory::
> +module.  Then run UML with the appropriate kernel command line
> +parameters::
>  
> -   ubd0=/path/to/uml/root/directory
> +   rootfstype=hostfs rw hostfs=uml_root_dir,xattrperm
>  
> -UML should then boot as it does normally.
> +You should have extended attributes supported and enabled on
> +your host filesystem since UML uses them to store correct file
> +permissions.

I'm not really sure we should basically say in the documentation that
"the way" to run it is with xattrperm? IOW, why not just add a new
paragraph that explains that (and how) you can add xattrperm, and what
it does?

johannes

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH 2/2] hostfs: store permissions in extended attributes

[Marko Petrović]

Signed-off-by: Marko Petrović <petrovicmarko2006@gmail.com>
---
 fs/hostfs/hostfs.h      |  5 ++-
 fs/hostfs/hostfs_kern.c | 23 ++++++++--
 fs/hostfs/hostfs_user.c | 96 +++++++++++++++++++++++++++++++++++++----
 3 files changed, 110 insertions(+), 14 deletions(-)

diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
index 69cb796f6270..9756303fc089 100644
--- a/fs/hostfs/hostfs.h
+++ b/fs/hostfs/hostfs.h
@@ -37,6 +37,7 @@
  * is on, and remove the appropriate bits from attr->ia_mode (attr is a
  * "struct iattr *"). -BlaisorBlade
  */
+extern int use_xattr;
 struct hostfs_timespec {
 	long long tv_sec;
 	long long tv_nsec;
@@ -83,11 +84,11 @@ extern int write_file(int fd, unsigned long long *offset, const char *buf,
 		      int len);
 extern int lseek_file(int fd, long long offset, int whence);
 extern int fsync_file(int fd, int datasync);
-extern int file_create(char *name, int mode);
+extern int file_create(char *name, int mode, uid_t uid, gid_t gid);
 extern int set_attr(const char *file, struct hostfs_iattr *attrs, int fd);
 extern int make_symlink(const char *from, const char *to);
 extern int unlink_file(const char *file);
-extern int do_mkdir(const char *file, int mode);
+extern int do_mkdir(const char *file, int mode, uid_t uid, gid_t gid);
 extern int hostfs_do_rmdir(const char *file);
 extern int do_mknod(const char *file, int mode, unsigned int major,
 		    unsigned int minor);
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 28b4f15c19eb..920d211d4e19 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -17,6 +17,7 @@
 #include <linux/writeback.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/uidgid.h>
 #include "hostfs.h"
 #include <init.h>
 #include <kern.h>
@@ -40,6 +41,7 @@ static struct kmem_cache *hostfs_inode_cache;
 /* Changed in hostfs_args before the kernel starts running */
 static char *root_ino = "";
 static int append = 0;
+int use_xattr;
 
 static const struct inode_operations hostfs_iops;
 static const struct inode_operations hostfs_dir_iops;
@@ -50,6 +52,7 @@ static int __init hostfs_args(char *options, int *add)
 {
 	char *ptr;
 
+	use_xattr = 0;
 	ptr = strchr(options, ',');
 	if (ptr != NULL)
 		*ptr++ = '\0';
@@ -64,6 +67,8 @@ static int __init hostfs_args(char *options, int *add)
 		if (*options != '\0') {
 			if (!strcmp(options, "append"))
 				append = 1;
+			else if (!strcmp(options, "xattrperm"))
+				use_xattr = 1;
 			else printf("hostfs_args - unsupported option - %s\n",
 				    options);
 		}
@@ -79,8 +84,10 @@ __uml_setup("hostfs=", hostfs_args,
 "    tree on the host.  If this isn't specified, then a user inside UML can\n"
 "    mount anything on the host that's accessible to the user that's running\n"
 "    it.\n"
-"    The only flag currently supported is 'append', which specifies that all\n"
-"    files opened by hostfs will be opened in append mode.\n\n"
+"    The only flags currently supported are 'append', which specifies that\n"
+"    all files opened by hostfs will be opened in append mode and 'xattrperm'\n"
+"    which specifies that permissions of files will be stored in extended\n"
+"    attributes.\n\n"
 );
 #endif
 
@@ -566,6 +573,8 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	struct inode *inode;
 	char *name;
 	int error, fd;
+	unsigned int currentuid;
+	unsigned int currentgid;
 
 	inode = hostfs_iget(dir->i_sb);
 	if (IS_ERR(inode)) {
@@ -578,7 +587,9 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	if (name == NULL)
 		goto out_put;
 
-	fd = file_create(name, mode & 0777);
+	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
+	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
+	fd = file_create(name, mode & 0777, currentuid, currentgid);
 	if (fd < 0)
 		error = fd;
 	else
@@ -677,10 +688,14 @@ static int hostfs_mkdir(struct mnt_idmap *idmap, struct inode *ino,
 {
 	char *file;
 	int err;
+	unsigned int currentuid;
+	unsigned int currentgid;
 
 	if ((file = dentry_name(dentry)) == NULL)
 		return -ENOMEM;
-	err = do_mkdir(file, mode);
+	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
+	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
+	err = do_mkdir(file, mode, currentuid, currentgid);
 	__putname(file);
 	return err;
 }
diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
index 5ecc4706172b..fdbd34b9add6 100644
--- a/fs/hostfs/hostfs_user.c
+++ b/fs/hostfs/hostfs_user.c
@@ -15,6 +15,7 @@
 #include <sys/types.h>
 #include <sys/vfs.h>
 #include <sys/syscall.h>
+#include <sys/xattr.h>
 #include "hostfs.h"
 #include <utime.h>
 
@@ -38,6 +39,82 @@ static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
 	p->min = os_minor(buf->st_rdev);
 }
 
+static int uml_chown(const char *pathname, unsigned int owner, unsigned int group)
+{
+	int status;
+
+	if (use_xattr) {
+		if (owner != -1) {
+			status = setxattr(pathname, "user.umluid", &owner,
+							sizeof(unsigned int), 0);
+			if (status < 0)
+				return status;
+		}
+		if (group != -1) {
+			status = setxattr(pathname, "user.umlgid", &owner,
+							sizeof(unsigned int), 0);
+			if (status < 0)
+				return status;
+		}
+		return 0;
+	} else {
+		return chown(pathname, owner, group);
+	}
+}
+
+static int uml_fchown(int fd, unsigned int owner, unsigned int group)
+{
+	int status;
+
+	if (use_xattr) {
+		if (owner != -1) {
+			status = fsetxattr(fd, "user.umluid", &owner,
+						sizeof(unsigned int), 0);
+			if (status < 0)
+				return status;
+		}
+		if (group != -1) {
+			status = fsetxattr(fd, "user.umlgid", &owner,
+						sizeof(unsigned int), 0);
+			if (status < 0)
+				return status;
+		}
+		return 0;
+	} else {
+		return fchown(fd, owner, group);
+	}
+}
+
+static int uml_chmod(const char *pathname, unsigned int mode)
+{
+	if (use_xattr)
+		return setxattr(pathname, "user.umlmode", &mode,
+						sizeof(unsigned int), 0);
+	return chmod(pathname, mode);
+}
+
+static int uml_fchmod(int fd, unsigned int mode)
+{
+	if (use_xattr)
+		return fsetxattr(fd, "user.umlmode", &mode,
+						sizeof(unsigned int), 0);
+	return fchmod(fd, mode);
+}
+
+static void read_permissions(const char *path, struct hostfs_stat *p)
+{
+	unsigned int mode, uid, gid;
+
+	if (!use_xattr)
+		return;
+	if (getxattr(path, "user.umlmode", &mode, sizeof(unsigned int)) != -1)
+		p->mode = mode;
+	if (getxattr(path, "user.umluid", &uid, sizeof(unsigned int)) != -1)
+		p->uid = uid;
+	if (getxattr(path, "user.umlgid", &gid, sizeof(unsigned int)) != -1)
+		p->gid = gid;
+}
+
 int stat_file(const char *path, struct hostfs_stat *p, int fd)
 {
 	struct stat64 buf;
@@ -49,6 +126,7 @@ int stat_file(const char *path, struct hostfs_stat *p, int fd)
 		return -errno;
 	}
 	stat64_to_hostfs(&buf, p);
+	read_permissions(path, p);
 	return 0;
 }
 
@@ -181,13 +259,14 @@ void close_dir(void *stream)
 	closedir(stream);
 }
 
-int file_create(char *name, int mode)
+int file_create(char *name, int mode, unsigned int uid, unsigned int gid)
 {
 	int fd;
 
 	fd = open64(name, O_CREAT | O_RDWR, mode);
 	if (fd < 0)
 		return -errno;
+	uml_chown(name, uid, gid);
 	return fd;
 }
 
@@ -199,25 +278,25 @@ int set_attr(const char *file, struct hostfs_iattr *attrs, int fd)
 
 	if (attrs->ia_valid & HOSTFS_ATTR_MODE) {
 		if (fd >= 0) {
-			if (fchmod(fd, attrs->ia_mode) != 0)
+			if (uml_fchmod(fd, attrs->ia_mode) != 0)
 				return -errno;
-		} else if (chmod(file, attrs->ia_mode) != 0) {
+		} else if (uml_chmod(file, attrs->ia_mode) != 0) {
 			return -errno;
 		}
 	}
 	if (attrs->ia_valid & HOSTFS_ATTR_UID) {
 		if (fd >= 0) {
-			if (fchown(fd, attrs->ia_uid, -1))
+			if (uml_fchown(fd, attrs->ia_uid, -1))
 				return -errno;
-		} else if (chown(file, attrs->ia_uid, -1)) {
+		} else if (uml_chown(file, attrs->ia_uid, -1)) {
 			return -errno;
 		}
 	}
 	if (attrs->ia_valid & HOSTFS_ATTR_GID) {
 		if (fd >= 0) {
-			if (fchown(fd, -1, attrs->ia_gid))
+			if (uml_fchown(fd, -1, attrs->ia_gid))
 				return -errno;
-		} else if (chown(file, -1, attrs->ia_gid)) {
+		} else if (uml_chown(file, -1, attrs->ia_gid)) {
 			return -errno;
 		}
 	}
@@ -294,13 +373,14 @@ int unlink_file(const char *file)
 	return 0;
 }
 
-int do_mkdir(const char *file, int mode)
+int do_mkdir(const char *file, int mode, unsigned int uid, unsigned int gid)
 {
 	int err;
 
 	err = mkdir(file, mode);
 	if (err)
 		return -errno;
+	uml_chown(file, uid, gid);
 	return 0;
 }
 
-- 
2.39.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH v2 2/2] hostfs: store permissions in extended attributes

[Marko Petrović]

Fix GID assignment error in uml_chown() and add support for correct
behavior when parent directory has SETGID bit.

Signed-off-by: Marko Petrović <petrovicmarko2006@gmail.com>
---
 fs/hostfs/hostfs.h      |   5 +-
 fs/hostfs/hostfs_kern.c |  23 +++++--
 fs/hostfs/hostfs_user.c | 142 +++++++++++++++++++++++++++++++++++++---
 3 files changed, 156 insertions(+), 14 deletions(-)

diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
index 69cb796f6270..9756303fc089 100644
--- a/fs/hostfs/hostfs.h
+++ b/fs/hostfs/hostfs.h
@@ -37,6 +37,7 @@
  * is on, and remove the appropriate bits from attr->ia_mode (attr is a
  * "struct iattr *"). -BlaisorBlade
  */
+extern int use_xattr;
 struct hostfs_timespec {
 	long long tv_sec;
 	long long tv_nsec;
@@ -83,11 +84,11 @@ extern int write_file(int fd, unsigned long long *offset, const char *buf,
 		      int len);
 extern int lseek_file(int fd, long long offset, int whence);
 extern int fsync_file(int fd, int datasync);
-extern int file_create(char *name, int mode);
+extern int file_create(char *name, int mode, uid_t uid, gid_t gid);
 extern int set_attr(const char *file, struct hostfs_iattr *attrs, int fd);
 extern int make_symlink(const char *from, const char *to);
 extern int unlink_file(const char *file);
-extern int do_mkdir(const char *file, int mode);
+extern int do_mkdir(const char *file, int mode, uid_t uid, gid_t gid);
 extern int hostfs_do_rmdir(const char *file);
 extern int do_mknod(const char *file, int mode, unsigned int major,
 		    unsigned int minor);
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 28b4f15c19eb..920d211d4e19 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -17,6 +17,7 @@
 #include <linux/writeback.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/uidgid.h>
 #include "hostfs.h"
 #include <init.h>
 #include <kern.h>
@@ -40,6 +41,7 @@ static struct kmem_cache *hostfs_inode_cache;
 /* Changed in hostfs_args before the kernel starts running */
 static char *root_ino = "";
 static int append = 0;
+int use_xattr;
 
 static const struct inode_operations hostfs_iops;
 static const struct inode_operations hostfs_dir_iops;
@@ -50,6 +52,7 @@ static int __init hostfs_args(char *options, int *add)
 {
 	char *ptr;
 
+	use_xattr = 0;
 	ptr = strchr(options, ',');
 	if (ptr != NULL)
 		*ptr++ = '\0';
@@ -64,6 +67,8 @@ static int __init hostfs_args(char *options, int *add)
 		if (*options != '\0') {
 			if (!strcmp(options, "append"))
 				append = 1;
+			else if (!strcmp(options, "xattrperm"))
+				use_xattr = 1;
 			else printf("hostfs_args - unsupported option - %s\n",
 				    options);
 		}
@@ -79,8 +84,10 @@ __uml_setup("hostfs=", hostfs_args,
 "    tree on the host.  If this isn't specified, then a user inside UML can\n"
 "    mount anything on the host that's accessible to the user that's running\n"
 "    it.\n"
-"    The only flag currently supported is 'append', which specifies that all\n"
-"    files opened by hostfs will be opened in append mode.\n\n"
+"    The only flags currently supported are 'append', which specifies that\n"
+"    all files opened by hostfs will be opened in append mode and 'xattrperm'\n"
+"    which specifies that permissions of files will be stored in extended\n"
+"    attributes.\n\n"
 );
 #endif
 
@@ -566,6 +573,8 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	struct inode *inode;
 	char *name;
 	int error, fd;
+	unsigned int currentuid;
+	unsigned int currentgid;
 
 	inode = hostfs_iget(dir->i_sb);
 	if (IS_ERR(inode)) {
@@ -578,7 +587,9 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	if (name == NULL)
 		goto out_put;
 
-	fd = file_create(name, mode & 0777);
+	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
+	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
+	fd = file_create(name, mode & 0777, currentuid, currentgid);
 	if (fd < 0)
 		error = fd;
 	else
@@ -677,10 +688,14 @@ static int hostfs_mkdir(struct mnt_idmap *idmap, struct inode *ino,
 {
 	char *file;
 	int err;
+	unsigned int currentuid;
+	unsigned int currentgid;
 
 	if ((file = dentry_name(dentry)) == NULL)
 		return -ENOMEM;
-	err = do_mkdir(file, mode);
+	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
+	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
+	err = do_mkdir(file, mode, currentuid, currentgid);
 	__putname(file);
 	return err;
 }
diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
index 5ecc4706172b..f2cc667ab0dd 100644
--- a/fs/hostfs/hostfs_user.c
+++ b/fs/hostfs/hostfs_user.c
@@ -15,6 +15,8 @@
 #include <sys/types.h>
 #include <sys/vfs.h>
 #include <sys/syscall.h>
+#include <sys/xattr.h>
+#include <sys/mman.h>
 #include "hostfs.h"
 #include <utime.h>
 
@@ -38,6 +40,118 @@ static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
 	p->min = os_minor(buf->st_rdev);
 }
 
+static int uml_chown(const char *pathname, unsigned int owner, unsigned int group)
+{
+	int status;
+
+	if (use_xattr) {
+		if (owner != -1) {
+			status = setxattr(pathname, "user.umluid", &owner,
+							sizeof(unsigned int), 0);
+			if (status < 0)
+				return status;
+		}
+		if (group != -1) {
+			status = setxattr(pathname, "user.umlgid", &group,
+							sizeof(unsigned int), 0);
+			if (status < 0)
+				return status;
+		}
+		return 0;
+	} else {
+		return chown(pathname, owner, group);
+	}
+}
+
+static int uml_fchown(int fd, unsigned int owner, unsigned int group)
+{
+	int status;
+
+	if (use_xattr) {
+		if (owner != -1) {
+			status = fsetxattr(fd, "user.umluid", &owner,
+						sizeof(unsigned int), 0);
+			if (status < 0)
+				return status;
+		}
+		if (group != -1) {
+			status = fsetxattr(fd, "user.umlgid", &group,
+						sizeof(unsigned int), 0);
+			if (status < 0)
+				return status;
+		}
+		return 0;
+	} else {
+		return fchown(fd, owner, group);
+	}
+}
+
+static int uml_chmod(const char *pathname, unsigned int mode)
+{
+	if (use_xattr)
+		return setxattr(pathname, "user.umlmode", &mode,
+						sizeof(unsigned int), 0);
+	return chmod(pathname, mode);
+}
+
+static int uml_fchmod(int fd, unsigned int mode)
+{
+	if (use_xattr)
+		return fsetxattr(fd, "user.umlmode", &mode,
+						sizeof(unsigned int), 0);
+	return fchmod(fd, mode);
+}
+
+static void read_permissions(const char *path, struct stat64 *p)
+{
+	unsigned int mode, uid, gid;
+
+	if (!use_xattr)
+		return;
+	if (getxattr(path, "user.umlmode", &mode, sizeof(unsigned int)) != -1)
+		p->st_mode = mode;
+	if (getxattr(path, "user.umluid", &uid, sizeof(unsigned int)) != -1)
+		p->st_uid = uid;
+	if (getxattr(path, "user.umlgid", &gid, sizeof(unsigned int)) != -1)
+		p->st_gid = gid;
+}
+
+// Remove double slash from the path
+static void fix_path(char *path)
+{
+	int i = 0;
+	int skip = 0;
+
+	while (path[i] != '\0') {
+		if (path[i] == '/' && path[i+1] == '/')
+			skip = 1;
+		path[i] = path[i+skip];
+		i++;
+	}
+}
+
+// path is in the form "rootfs//var/abc"
+static long is_set_gid(const char *path)
+{
+	int i = strlen(path) + 1;
+	char *parent = mmap(NULL, i, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
+	struct stat64 buf = { 0 };
+
+	strcpy(parent, path);
+	i = i - 3;
+	while (parent[i] != '/')
+		i--;
+	parent[i] = '\0';
+	fix_path(parent);
+
+	stat64(parent, &buf);
+	read_permissions(parent, &buf);
+	munmap(parent, strlen(path) + 1);
+	if (buf.st_mode & S_ISGID)
+		return buf.st_gid;
+	return -1;
+}
+
 int stat_file(const char *path, struct hostfs_stat *p, int fd)
 {
 	struct stat64 buf;
@@ -48,6 +162,7 @@ int stat_file(const char *path, struct hostfs_stat *p, int fd)
 	} else if (lstat64(path, &buf) < 0) {
 		return -errno;
 	}
+	read_permissions(path, &buf);
 	stat64_to_hostfs(&buf, p);
 	return 0;
 }
@@ -181,13 +296,19 @@ void close_dir(void *stream)
 	closedir(stream);
 }
 
-int file_create(char *name, int mode)
+int file_create(char *name, int mode, unsigned int uid, unsigned int gid)
 {
 	int fd;
+	long ret;
 
 	fd = open64(name, O_CREAT | O_RDWR, mode);
 	if (fd < 0)
 		return -errno;
+
+	ret = is_set_gid(name);
+	if (ret != -1)
+		gid = ret;
+	uml_chown(name, uid, gid);
 	return fd;
 }
 
@@ -199,25 +320,25 @@ int set_attr(const char *file, struct hostfs_iattr *attrs, int fd)
 
 	if (attrs->ia_valid & HOSTFS_ATTR_MODE) {
 		if (fd >= 0) {
-			if (fchmod(fd, attrs->ia_mode) != 0)
+			if (uml_fchmod(fd, attrs->ia_mode) != 0)
 				return -errno;
-		} else if (chmod(file, attrs->ia_mode) != 0) {
+		} else if (uml_chmod(file, attrs->ia_mode) != 0) {
 			return -errno;
 		}
 	}
 	if (attrs->ia_valid & HOSTFS_ATTR_UID) {
 		if (fd >= 0) {
-			if (fchown(fd, attrs->ia_uid, -1))
+			if (uml_fchown(fd, attrs->ia_uid, -1))
 				return -errno;
-		} else if (chown(file, attrs->ia_uid, -1)) {
+		} else if (uml_chown(file, attrs->ia_uid, -1)) {
 			return -errno;
 		}
 	}
 	if (attrs->ia_valid & HOSTFS_ATTR_GID) {
 		if (fd >= 0) {
-			if (fchown(fd, -1, attrs->ia_gid))
+			if (uml_fchown(fd, -1, attrs->ia_gid))
 				return -errno;
-		} else if (chown(file, -1, attrs->ia_gid)) {
+		} else if (uml_chown(file, -1, attrs->ia_gid)) {
 			return -errno;
 		}
 	}
@@ -294,13 +415,18 @@ int unlink_file(const char *file)
 	return 0;
 }
 
-int do_mkdir(const char *file, int mode)
+int do_mkdir(const char *file, int mode, unsigned int uid, unsigned int gid)
 {
 	int err;
+	long ret;
 
 	err = mkdir(file, mode);
 	if (err)
 		return -errno;
+	ret = is_set_gid(file);
+	if (ret != -1)
+		gid = ret;
+	uml_chown(file, uid, gid);
 	return 0;
 }
 
-- 
2.39.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 2/2] hostfs: store permissions in extended attributes

[Johannes Berg]

On Fri, 2023-04-14 at 04:33 +0200, Marko Petrović wrote:
> Fix GID assignment error in uml_chown() and add support for correct
> behavior when parent directory has SETGID bit.


That was the change for 'v2' I guess, but you should document here what
the new code does and why, probably a good chunk of your cover letter
could be here :-)

> +++ b/fs/hostfs/hostfs.h
> @@ -37,6 +37,7 @@
>   * is on, and remove the appropriate bits from attr->ia_mode (attr is a
>   * "struct iattr *"). -BlaisorBlade
>   */
> +extern int use_xattr;


That seems like an odd place to put it - the comment above is surely for
the hostfs_timespec?

> --- a/fs/hostfs/hostfs_kern.c
> +++ b/fs/hostfs/hostfs_kern.c
> @@ -17,6 +17,7 @@
>  #include <linux/writeback.h>
>  #include <linux/mount.h>
>  #include <linux/namei.h>
> +#include <linux/uidgid.h>
>  #include "hostfs.h"
>  #include <init.h>
>  #include <kern.h>
> @@ -40,6 +41,7 @@ static struct kmem_cache *hostfs_inode_cache;
>  /* Changed in hostfs_args before the kernel starts running */
>  static char *root_ino = "";
>  static int append = 0;
> +int use_xattr;

Not relevant just _yet_, but FYI, I have a patch somewhere to build the
_user.c part always into the kernel image, and then this needs to be
over there and exported for the module, I think.

(But I need to see what's the state of my patch)

> @@ -578,7 +587,9 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
>  	if (name == NULL)
>  		goto out_put;
>  
> -	fd = file_create(name, mode & 0777);
> +	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
> +	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
> +	fd = file_create(name, mode & 0777, currentuid, currentgid);
>  	if (fd < 0)
>  		error = fd;
>  	else

That probably somehow interacts with the idmapping, +Glenn & Christian.

Not saying it's wrong, but I don't understand it.


> @@ -677,10 +688,14 @@ static int hostfs_mkdir(struct mnt_idmap *idmap, struct inode *ino,
>  {
>  	char *file;
>  	int err;
> +	unsigned int currentuid;
> +	unsigned int currentgid;
>  
>  	if ((file = dentry_name(dentry)) == NULL)
>  		return -ENOMEM;
> -	err = do_mkdir(file, mode);
> +	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
> +	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
> +	err = do_mkdir(file, mode, currentuid, currentgid);

Here too.

> +static int uml_chown(const char *pathname, unsigned int owner, unsigned int group)
> +{
> +	int status;

Maybe just call that the more commonly used 'ret'? It does seem to
follow kernel conventions (negative error codes, 0 for success).

> +
> +	if (use_xattr) {
> +		if (owner != -1) {
> +			status = setxattr(pathname, "user.umluid", &owner,
> +							sizeof(unsigned int), 0);

Indentation _seems_ to be off a bit here, but could be my mail client.

> +			if (status < 0)
> +				return status;
> +		}
> +		if (group != -1) {
> +			status = setxattr(pathname, "user.umlgid", &group,
> +							sizeof(unsigned int), 0);
> +			if (status < 0)
> +				return status;
> +		}
> +		return 0;

The logic here means you can partially succeed with chown even when this
fails, maybe that's not great?

Also, storing the value of 'owner' and 'group' as binary in host-endian
format makes this needlessly unportable, though we don't really have UML
for anything but x86 at this point ...

Maybe for both it'd be better to do something like a 'user.umlperms'
attribute that contains '<mode>,<uid>,<gid>' as a string? That also
makes it easier to understand if you end up looking at it from the
outside.

Additionally - and again that's not relevant now I _think_ - we'd have
to filter this attribute if hostfs ever supports xattrs inside, no?
Assuming it would just push them into the same xattr outside, which
seems reasonable though. Or maybe in that case some name translation
should happen? I don't think anyone wants to run nested UML with hostfs
though ...


> +	} else {
> +		return chown(pathname, owner, group);

You returned above, so don't really need the "else" here.


> +// Remove double slash from the path

Please don't use // comments.

> +static void fix_path(char *path)
> +{
> +	int i = 0;
> +	int skip = 0;
> +
> +	while (path[i] != '\0') {
> +		if (path[i] == '/' && path[i+1] == '/')
> +			skip = 1;
> +		path[i] = path[i+skip];
> +		i++;
> +	}
> +}

Why is this needed? I don't think the host should have an issue with
doubled slashes?

> +// path is in the form "rootfs//var/abc"
> +static long is_set_gid(const char *path)
> +{
> +	int i = strlen(path) + 1;
> +	char *parent = mmap(NULL, i, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);

Why is this mmap()? We have uml_kmalloc() and I think that should work?

> +	struct stat64 buf = { 0 };
> +
> +	strcpy(parent, path);
> +	i = i - 3;

why -3?

> +	while (parent[i] != '/')
> +		i--;
> +	parent[i] = '\0';
> +	fix_path(parent);
> +
> +	stat64(parent, &buf);

Is it even worth doing the stat on the host? if the S_ISGID is set
there, it'll be done anyway by the host no? IOW, don't you only need to
do the handling for 'internal' xattr permissions?

>  	if (attrs->ia_valid & HOSTFS_ATTR_MODE) {
>  		if (fd >= 0) {
> -			if (fchmod(fd, attrs->ia_mode) != 0)
> +			if (uml_fchmod(fd, attrs->ia_mode) != 0)
>  				return -errno;

The error handling here feels confusing now - it would seem to me that
it's better to make it so that uml_fchmod() already returns the -errno
in the appropriate places, and then use the returned value here?

Otherwise there's just so much more code between the call and reading
errno that it gets harder to follow, IMHO.

johannes

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 2/2] hostfs: store permissions in extended attributes

[Marko Petrović]

On Fri Apr 14, 2023 at 9:40 AM CEST, Johannes Berg wrote:
> On Fri, 2023-04-14 at 04:33 +0200, Marko Petrović wrote:
> > Fix GID assignment error in uml_chown() and add support for correct
> > behavior when parent directory has SETGID bit.
>
>
> That was the change for 'v2' I guess, but you should document here what
> the new code does and why, probably a good chunk of your cover letter
> could be here :-)
Hello! First, thank you for your feedback!
Ok, when sending the PATCH v3, I will document entire patch here.
>
> > +++ b/fs/hostfs/hostfs.h
> > @@ -37,6 +37,7 @@
> >   * is on, and remove the appropriate bits from attr->ia_mode (attr is a
> >   * "struct iattr *"). -BlaisorBlade
> >   */
> > +extern int use_xattr;
>
>
> That seems like an odd place to put it - the comment above is surely for
> the hostfs_timespec?
Actually it appears to be talking about the missing defines of
ATTR_KILL_SUID and ATTR_KILL_SGID, unrelated to the struct hostfs_timespec.
As for why I put it there, I thought it would be nice not to mix
function declarations with variable declarations.
>
> > --- a/fs/hostfs/hostfs_kern.c
> > +++ b/fs/hostfs/hostfs_kern.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/writeback.h>
> >  #include <linux/mount.h>
> >  #include <linux/namei.h>
> > +#include <linux/uidgid.h>
> >  #include "hostfs.h"
> >  #include <init.h>
> >  #include <kern.h>
> > @@ -40,6 +41,7 @@ static struct kmem_cache *hostfs_inode_cache;
> >  /* Changed in hostfs_args before the kernel starts running */
> >  static char *root_ino = "";
> >  static int append = 0;
> > +int use_xattr;
>
> Not relevant just _yet_, but FYI, I have a patch somewhere to build the
> _user.c part always into the kernel image, and then this needs to be
> over there and exported for the module, I think.
>
> (But I need to see what's the state of my patch)
Ok. In that case, I can add EXPORT_SYMBOL() for these variables.
>
> > @@ -578,7 +587,9 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
> >  	if (name == NULL)
> >  		goto out_put;
> >  
> > -	fd = file_create(name, mode & 0777);
> > +	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
> > +	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
> > +	fd = file_create(name, mode & 0777, currentuid, currentgid);
> >  	if (fd < 0)
> >  		error = fd;
> >  	else
>
> That probably somehow interacts with the idmapping, +Glenn & Christian.
>
> Not saying it's wrong, but I don't understand it.
If I understood documentation correctly, filesystems should store userspace
ids and kernel performs translation of userspace id to kernel id when
file is read from the disk.
On the other hand, the variables euid and egid in struct cred are of
type kuid_t and kgid_t so this here performs reverse translation
(from kernel id to userspace id) based on the process' current user
namespace. Then the resulting userspace ids are sent to file_create()
to be stored on the disk.
If I have made some misunderstanding and this code has any error,
I apologise and please inform me so that I can fix it.
>
> > @@ -677,10 +688,14 @@ static int hostfs_mkdir(struct mnt_idmap *idmap, struct inode *ino,
> >  {
> >  	char *file;
> >  	int err;
> > +	unsigned int currentuid;
> > +	unsigned int currentgid;
> >  
> >  	if ((file = dentry_name(dentry)) == NULL)
> >  		return -ENOMEM;
> > -	err = do_mkdir(file, mode);
> > +	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
> > +	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
> > +	err = do_mkdir(file, mode, currentuid, currentgid);
>
> Here too.
>
> > +static int uml_chown(const char *pathname, unsigned int owner, unsigned int group)
> > +{
> > +	int status;
>
> Maybe just call that the more commonly used 'ret'? It does seem to
> follow kernel conventions (negative error codes, 0 for success).
Ok. Will be changed.
>
> > +
> > +	if (use_xattr) {
> > +		if (owner != -1) {
> > +			status = setxattr(pathname, "user.umluid", &owner,
> > +							sizeof(unsigned int), 0);
>
> Indentation _seems_ to be off a bit here, but could be my mail client.
I just checked, it is 82 columns wide. Sorry about that, will fix it.
>
> > +			if (status < 0)
> > +				return status;
> > +		}
> > +		if (group != -1) {
> > +			status = setxattr(pathname, "user.umlgid", &group,
> > +							sizeof(unsigned int), 0);
> > +			if (status < 0)
> > +				return status;
> > +		}
> > +		return 0;
>
> The logic here means you can partially succeed with chown even when this
> fails, maybe that's not great?
>
> Also, storing the value of 'owner' and 'group' as binary in host-endian
> format makes this needlessly unportable, though we don't really have UML
> for anything but x86 at this point ...
>
> Maybe for both it'd be better to do something like a 'user.umlperms'
> attribute that contains '<mode>,<uid>,<gid>' as a string? That also
> makes it easier to understand if you end up looking at it from the
> outside.
>
> Additionally - and again that's not relevant now I _think_ - we'd have
> to filter this attribute if hostfs ever supports xattrs inside, no?
> Assuming it would just push them into the same xattr outside, which
> seems reasonable though. Or maybe in that case some name translation
> should happen? I don't think anyone wants to run nested UML with hostfs
> though ...
>
Thank you for your suggestion! I will change format of stored
permissions to one string stored in one extended attribute.

Regarding nested UMLs using hostfs, a simple append of some char
(or string?) to the attribute name when it starts with user.umlperms
should solve the issue. Then first UML would access user.umlperms, UML
inside it would access user.umlperms1, UML inside that UML would access
user.umlperms11, etc. as seen by the host.
>
> > +	} else {
> > +		return chown(pathname, owner, group);
>
> You returned above, so don't really need the "else" here.
>
>
> > +// Remove double slash from the path
>
> Please don't use // comments.
>
> > +static void fix_path(char *path)
> > +{
> > +	int i = 0;
> > +	int skip = 0;
> > +
> > +	while (path[i] != '\0') {
> > +		if (path[i] == '/' && path[i+1] == '/')
> > +			skip = 1;
> > +		path[i] = path[i+skip];
> > +		i++;
> > +	}
> > +}
>
> Why is this needed? I don't think the host should have an issue with
> doubled slashes?
It appears that it really isn't needed. It was a leftover from earlier
attempt to fix some problem and then I assumed that maybe functions in
the rest of the code work using file descriptors so I left this just in
case.
Anyway, will remove it.
>
> > +// path is in the form "rootfs//var/abc"
> > +static long is_set_gid(const char *path)
> > +{
> > +	int i = strlen(path) + 1;
> > +	char *parent = mmap(NULL, i, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0);
>
> Why is this mmap()? We have uml_kmalloc() and I think that should work?
That was a temporary fix... I couldn't find identifier for function for
memory allocation in this part of the kernel. That will be changed,
thanks for information.
>
> > +	struct stat64 buf = { 0 };
> > +
> > +	strcpy(parent, path);
> > +	i = i - 3;
>
> why -3?
i is initially strlen(path) + 1, i.e. the length of the whole string
including the null byte. Then i-1 is the index of the null byte and i-2
is the index of the last character, but last charcter cannot be the
slash that separates file from it's parent directory, so search starts
from i-3.
>
> > +	while (parent[i] != '/')
> > +		i--;
> > +	parent[i] = '\0';
> > +	fix_path(parent);
> > +
> > +	stat64(parent, &buf);
>
> Is it even worth doing the stat on the host? if the S_ISGID is set
> there, it'll be done anyway by the host no? IOW, don't you only need to
> do the handling for 'internal' xattr permissions?
>
Yes, it will be done by the host but it will then be overridden by the
permissions in xattr so UML would present wrong GID to UML processes.
To fix that, UML does stat on the file to determine whether S_ISGID is
set there so that xattr permissions reflect that.

> >  	if (attrs->ia_valid & HOSTFS_ATTR_MODE) {
> >  		if (fd >= 0) {
> > -			if (fchmod(fd, attrs->ia_mode) != 0)
> > +			if (uml_fchmod(fd, attrs->ia_mode) != 0)
> >  				return -errno;
>
> The error handling here feels confusing now - it would seem to me that
> it's better to make it so that uml_fchmod() already returns the -errno
> in the appropriate places, and then use the returned value here?
>
> Otherwise there's just so much more code between the call and reading
> errno that it gets harder to follow, IMHO.
Ok. That will be fixed.
>
> johannes
Best regards,
Marko Petrovic


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 2/2] hostfs: store permissions in extended attributes

[Johannes Berg]

On Fri, 2023-04-14 at 19:19 +0200, Marko Petrović wrote:
> > > +++ b/fs/hostfs/hostfs.h
> > > @@ -37,6 +37,7 @@
> > >   * is on, and remove the appropriate bits from attr->ia_mode (attr is a
> > >   * "struct iattr *"). -BlaisorBlade
> > >   */
> > > +extern int use_xattr;
> > 
> > 
> > That seems like an odd place to put it - the comment above is surely for
> > the hostfs_timespec?
> Actually it appears to be talking about the missing defines of
> ATTR_KILL_SUID and ATTR_KILL_SGID, unrelated to the struct hostfs_timespec.
> As for why I put it there, I thought it would be nice not to mix
> function declarations with variable declarations.

Oh, OK, sorry. Maybe just add a couple of blank line around it then so
it doesn't look so grouped? :)
> > 
> > Not relevant just _yet_, but FYI, I have a patch somewhere to build the
> > _user.c part always into the kernel image, and then this needs to be
> > over there and exported for the module, I think.
> > 
> > (But I need to see what's the state of my patch)
> Ok. In that case, I can add EXPORT_SYMBOL() for these variables.

Right. FWIW, the patch series is here:
https://patchwork.ozlabs.org/project/linux-um/list/?series=341340

I hope Richard will apply these soon.

> > > @@ -578,7 +587,9 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
> > >  	if (name == NULL)
> > >  		goto out_put;
> > >  
> > > -	fd = file_create(name, mode & 0777);
> > > +	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
> > > +	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
> > > +	fd = file_create(name, mode & 0777, currentuid, currentgid);
> > >  	if (fd < 0)
> > >  		error = fd;
> > >  	else
> > 
> > That probably somehow interacts with the idmapping, +Glenn & Christian.
> > 
> > Not saying it's wrong, but I don't understand it.

> If I understood documentation correctly, filesystems should store userspace
> ids and kernel performs translation of userspace id to kernel id when
> file is read from the disk.

Makes sense.

> On the other hand, the variables euid and egid in struct cred are of
> type kuid_t and kgid_t so this here performs reverse translation
> (from kernel id to userspace id) based on the process' current user
> namespace. Then the resulting userspace ids are sent to file_create()
> to be stored on the disk.

OK :)

> If I have made some misunderstanding and this code has any error,
> I apologise and please inform me so that I can fix it.

Oh, no, I really just don't know! Just noticed that there was other work
in this area.

> > 
> > > +
> > > +	if (use_xattr) {
> > > +		if (owner != -1) {
> > > +			status = setxattr(pathname, "user.umluid", &owner,
> > > +							sizeof(unsigned int), 0);
> > 
> > Indentation _seems_ to be off a bit here, but could be my mail client.
> I just checked, it is 82 columns wide. Sorry about that, will fix it.

Ah, that's not what I meant, I meant the indentation should be only to
just after the ( on the next line.

> Regarding nested UMLs using hostfs, a simple append of some char
> (or string?) to the attribute name when it starts with user.umlperms
> should solve the issue. Then first UML would access user.umlperms, UML
> inside it would access user.umlperms1, UML inside that UML would access
> user.umlperms11, etc. as seen by the host.

Not sure that seems like a good idea, then wouldn't you read the
permissions depending on the nesting level you're running on or
something? How would that even work?

Anyway it all doesn't work since hostfs itself doesn't seem to support
xattrs now, so you can't store xattrs in it, so only then would we have
to think about it.

And I think at that point we'd probably want to do some mangling like

	user.xyz
	security.xyz

attributes inside hostfs getting stored into

	user.uml.user.xyz
	user.uml.security.xyz

on the host filesystem, or such. And then it wouldn't conflict, and if
you nested, you'd get "user.uml.user.umlcred" (or what you chose now),
you'd just limit the nesting here on the depth of the xattr name ;)

Nothing we need to think about too much right now, I guess.

> > 
> > > +	struct stat64 buf = { 0 };
> > > +
> > > +	strcpy(parent, path);
> > > +	i = i - 3;
> > 
> > why -3?
> i is initially strlen(path) + 1, i.e. the length of the whole string
> including the null byte. Then i-1 is the index of the null byte and i-2
> is the index of the last character, but last charcter cannot be the
> slash that separates file from it's parent directory, so search starts
> from i-3.

Would be good to add a comment with that I think :)


> > > +	while (parent[i] != '/')
> > > +		i--;
> > > +	parent[i] = '\0';
> > > +	fix_path(parent);
> > > +
> > > +	stat64(parent, &buf);
> > 
> > Is it even worth doing the stat on the host? if the S_ISGID is set
> > there, it'll be done anyway by the host no? IOW, don't you only need to
> > do the handling for 'internal' xattr permissions?
> > 
> Yes, it will be done by the host but it will then be overridden by the
> permissions in xattr so UML would present wrong GID to UML processes.
> To fix that, UML does stat on the file to determine whether S_ISGID is
> set there so that xattr permissions reflect that.

Ah right, makes sense.

johannes

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 2/2] hostfs: store permissions in extended attributes

[Marko Petrović]

On Tue Apr 18, 2023 at 10:26 AM CEST, Johannes Berg wrote:
Hello,
Thank you for your review!
> On Fri, 2023-04-14 at 19:19 +0200, Marko Petrović wrote:
> > > > +++ b/fs/hostfs/hostfs.h
> > > > @@ -37,6 +37,7 @@
> > > >   * is on, and remove the appropriate bits from attr->ia_mode (attr is a
> > > >   * "struct iattr *"). -BlaisorBlade
> > > >   */
> > > > +extern int use_xattr;
> > > 
> > > 
> > > That seems like an odd place to put it - the comment above is surely for
> > > the hostfs_timespec?
> > Actually it appears to be talking about the missing defines of
> > ATTR_KILL_SUID and ATTR_KILL_SGID, unrelated to the struct hostfs_timespec.
> > As for why I put it there, I thought it would be nice not to mix
> > function declarations with variable declarations.
>
> Oh, OK, sorry. Maybe just add a couple of blank line around it then so
> it doesn't look so grouped? :)
Ok. Will add them. :)

> > Regarding nested UMLs using hostfs, a simple append of some char
> > (or string?) to the attribute name when it starts with user.umlperms
> > should solve the issue. Then first UML would access user.umlperms, UML
> > inside it would access user.umlperms1, UML inside that UML would access
> > user.umlperms11, etc. as seen by the host.
>
> Not sure that seems like a good idea, then wouldn't you read the
> permissions depending on the nesting level you're running on or
> something? How would that even work?
Yes, that was the idea, kind of. It seemed like a good idea for each UML
on each nesting level to have it's own permission attributes and not
mess with host's permissions.
>
> Anyway it all doesn't work since hostfs itself doesn't seem to support
> xattrs now, so you can't store xattrs in it, so only then would we have
> to think about it.
>
> And I think at that point we'd probably want to do some mangling like
>
> 	user.xyz
> 	security.xyz
>
> attributes inside hostfs getting stored into
>
> 	user.uml.user.xyz
> 	user.uml.security.xyz
>
> on the host filesystem, or such. And then it wouldn't conflict, and if
> you nested, you'd get "user.uml.user.umlcred" (or what you chose now),
> you'd just limit the nesting here on the depth of the xattr name ;)
>
> Nothing we need to think about too much right now, I guess.
If I understood correctly, that also accomplishes more or less the same
thing, it just prepends the "user.uml." string to the original attribute
instead of appending some char so there would be user.umlcred for the
first UML, user.uml.user.umlcred for the second UML,
user.uml.user.uml.user.umlcred for the third UML, etc.

Apart from longer names, it seems fine. In any case, as you said, we will
think this through when hostfs gets extended attributes support.

> > > 
> > > > +	struct stat64 buf = { 0 };
> > > > +
> > > > +	strcpy(parent, path);
> > > > +	i = i - 3;
> > > 
> > > why -3?
> > i is initially strlen(path) + 1, i.e. the length of the whole string
> > including the null byte. Then i-1 is the index of the null byte and i-2
> > is the index of the last character, but last charcter cannot be the
> > slash that separates file from it's parent directory, so search starts
> > from i-3.
>
> Would be good to add a comment with that I think :)
>
Ok. Will add it.

Best Regards,
Marko Petrović

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 2/2] hostfs: store permissions in extended attributes

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Marko Petrović" <petrovicmarko2006@gmail.com>
> +static int uml_chown(const char *pathname, unsigned int owner, unsigned int
> group)

Is there a specific reason why you are not using uid_t and gid_t?

> +{
> +	int status;
> +
> +	if (use_xattr) {
> +		if (owner != -1) {

Doesn't -1 clash with the uid space?
Same for gid.

> +			status = setxattr(pathname, "user.umluid", &owner,
> +							sizeof(unsigned int), 0);


I'd prefer storing the values platform independent. e.g. in __le32. 
Just to be future prove.

Thanks,
//richard

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 2/2] hostfs: store permissions in extended attributes

[Marko Petrović]

On Fri Apr 14, 2023 at 12:54 PM CEST, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Marko Petrović" <petrovicmarko2006@gmail.com>
> > +static int uml_chown(const char *pathname, unsigned int owner, unsigned int
> > group)
>
> Is there a specific reason why you are not using uid_t and gid_t?
>
> > +{
> > +	int status;
> > +
> > +	if (use_xattr) {
> > +		if (owner != -1) {
>
> Doesn't -1 clash with the uid space?
> Same for gid.
>
> > +			status = setxattr(pathname, "user.umluid", &owner,
> > +							sizeof(unsigned int), 0);
>
>
> I'd prefer storing the values platform independent. e.g. in __le32. 
> Just to be future prove.
>
> Thanks,
> //richard
Hello,
Thank you for your feedback!

There is no specific reason for preferring one over the other between
uid_t/gid_t and unsigned int. I will change those types back to uid_t
and gid_t.

About using -1 for uid and gid, in documentation for chown it is stated
that -1 shall be used if the desired value should not be changed, since
normally chown(2) accepts both uid and gid for changing in one system
call.
In the concrete implementation, since the underlying type is unsigned
int, the -1 will be written in 2's complement and then treated as an
unsigned positive number. For the size of 4 bytes for unsigned int, the
resulting number is 4294967295. That is also the maximum possible number
of users on Linux, but Linux user IDs go from 0 to 4294967294 so that
last one is outside UID space and reserved as "don't change" flag for
chown(2).

In regard to the store of values in platform independent way, in my
humble opinion, Johannes' recommendation may be preferred (storing all
permissions in one string) because besides this it also solves the
problem of one setxattr() succeding and other failing (for whatever
reason may that happen) in uml_chown().

Best regards,
Marko Petrović

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v2 2/2] hostfs: store permissions in extended attributes

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Marko Petrović" <petrovicmarko2006@gmail.com>
> About using -1 for uid and gid, in documentation for chown it is stated
> that -1 shall be used if the desired value should not be changed, since
> normally chown(2) accepts both uid and gid for changing in one system
> call.
> In the concrete implementation, since the underlying type is unsigned
> int, the -1 will be written in 2's complement and then treated as an
> unsigned positive number. For the size of 4 bytes for unsigned int, the
> resulting number is 4294967295. That is also the maximum possible number
> of users on Linux, but Linux user IDs go from 0 to 4294967294 so that
> last one is outside UID space and reserved as "don't change" flag for
> chown(2).

You're totally right. I should have checked the manpage before.
 
> In regard to the store of values in platform independent way, in my
> humble opinion, Johannes' recommendation may be preferred (storing all
> permissions in one string) because besides this it also solves the
> problem of one setxattr() succeding and other failing (for whatever
> reason may that happen) in uml_chown().

Storing them as string is also fine by me. :-)

Thanks,
//richard

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

[PATCH v3 2/2] hostfs: store permissions in extended attributes

[Marko Petrović]

This patch adds support for xattrperm hostfs kernel command line option
which enables the use of extended attributes for storing permissions by
default on all mounted hostfs filesystems.

The support for per-mountpoint xattrperm/noxattrperm is added.

'struct super_block -> s_fs_info' now points to 'struct hostfs_fs_info'.
All code that relied on s_fs_info pointing to a string is changed to use
the same string stored inside 'struct hostfs_fs_info'. This allows easy
extensions of the super_block data in the future for storing mount
options.

Function hostfs_parse_options() is added for parsing the string of
mount options and storing them in 'struct hostfs_fs_info'.

When xattrperm is enabled, filesystem stores permissions in a
human-readable string in attributes user.umlmode (for mode) and
user.umlcred (for uid and gid).

Signed-off-by: Marko Petrović <petrovicmarko2006@gmail.com>
---
 fs/hostfs/hostfs.h      |  13 ++-
 fs/hostfs/hostfs_kern.c | 129 +++++++++++++++++----
 fs/hostfs/hostfs_user.c | 242 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 340 insertions(+), 44 deletions(-)

diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
index 69cb796f6270..03f773b7c423 100644
--- a/fs/hostfs/hostfs.h
+++ b/fs/hostfs/hostfs.h
@@ -37,6 +37,7 @@
  * is on, and remove the appropriate bits from attr->ia_mode (attr is a
  * "struct iattr *"). -BlaisorBlade
  */
+extern int use_xattr;
 struct hostfs_timespec {
 	long long tv_sec;
 	long long tv_nsec;
@@ -67,7 +68,8 @@ struct hostfs_stat {
 	unsigned int min;
 };
 
-extern int stat_file(const char *path, struct hostfs_stat *p, int fd);
+extern int stat_file(const char *path, struct hostfs_stat *p, int fd,
+							int mnt_use_xattr);
 extern int access_file(char *path, int r, int w, int x);
 extern int open_file(char *path, int r, int w, int append);
 extern void *open_dir(char *path, int *err_out);
@@ -83,11 +85,14 @@ extern int write_file(int fd, unsigned long long *offset, const char *buf,
 		      int len);
 extern int lseek_file(int fd, long long offset, int whence);
 extern int fsync_file(int fd, int datasync);
-extern int file_create(char *name, int mode);
-extern int set_attr(const char *file, struct hostfs_iattr *attrs, int fd);
+extern int file_create(char *name, int mode, uid_t uid, gid_t gid,
+							int mnt_use_xattr);
+extern int set_attr(const char *file, struct hostfs_iattr *attrs, int fd,
+							    int mnt_use_xattr);
 extern int make_symlink(const char *from, const char *to);
 extern int unlink_file(const char *file);
-extern int do_mkdir(const char *file, int mode);
+extern int do_mkdir(const char *file, int mode, uid_t uid, gid_t gid,
+							int mnt_use_xattr);
 extern int hostfs_do_rmdir(const char *file);
 extern int do_mknod(const char *file, int mode, unsigned int major,
 		    unsigned int minor);
diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 28b4f15c19eb..a2afe70abf14 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -17,6 +17,7 @@
 #include <linux/writeback.h>
 #include <linux/mount.h>
 #include <linux/namei.h>
+#include <linux/uidgid.h>
 #include "hostfs.h"
 #include <init.h>
 #include <kern.h>
@@ -28,6 +29,11 @@ struct hostfs_inode_info {
 	struct mutex open_mutex;
 };
 
+struct hostfs_fs_info {
+	char host_root_path[PATH_MAX+1];
+	int use_xattr;
+};
+
 static inline struct hostfs_inode_info *HOSTFS_I(struct inode *inode)
 {
 	return list_entry(inode, struct hostfs_inode_info, vfs_inode);
@@ -64,6 +70,8 @@ static int __init hostfs_args(char *options, int *add)
 		if (*options != '\0') {
 			if (!strcmp(options, "append"))
 				append = 1;
+			else if (!strcmp(options, "xattrperm"))
+				use_xattr = 1;
 			else printf("hostfs_args - unsupported option - %s\n",
 				    options);
 		}
@@ -79,18 +87,22 @@ __uml_setup("hostfs=", hostfs_args,
 "    tree on the host.  If this isn't specified, then a user inside UML can\n"
 "    mount anything on the host that's accessible to the user that's running\n"
 "    it.\n"
-"    The only flag currently supported is 'append', which specifies that all\n"
-"    files opened by hostfs will be opened in append mode.\n\n"
+"    The only flags currently supported are 'append', which specifies that\n"
+"    all files opened by hostfs will be opened in append mode and 'xattrperm'\n"
+"    which specifies that permissions of files will be stored in extended\n"
+"    attributes.\n\n"
 );
 #endif
 
 static char *__dentry_name(struct dentry *dentry, char *name)
 {
 	char *p = dentry_path_raw(dentry, name, PATH_MAX);
+	struct hostfs_fs_info *sb_data;
 	char *root;
 	size_t len;
 
-	root = dentry->d_sb->s_fs_info;
+	sb_data = dentry->d_sb->s_fs_info;
+	root = sb_data->host_root_path;
 	len = strlen(root);
 	if (IS_ERR(p)) {
 		__putname(name);
@@ -203,8 +215,10 @@ static int hostfs_statfs(struct dentry *dentry, struct kstatfs *sf)
 	long long f_bavail;
 	long long f_files;
 	long long f_ffree;
+	struct hostfs_fs_info *sb_data;
 
-	err = do_statfs(dentry->d_sb->s_fs_info,
+	sb_data = dentry->d_sb->s_fs_info;
+	err = do_statfs(sb_data->host_root_path,
 			&sf->f_bsize, &f_blocks, &f_bfree, &f_bavail, &f_files,
 			&f_ffree, &sf->f_fsid, sizeof(sf->f_fsid),
 			&sf->f_namelen);
@@ -250,15 +264,21 @@ static void hostfs_free_inode(struct inode *inode)
 
 static int hostfs_show_options(struct seq_file *seq, struct dentry *root)
 {
-	const char *root_path = root->d_sb->s_fs_info;
+	struct hostfs_fs_info *sb_data = root->d_sb->s_fs_info;
 	size_t offset = strlen(root_ino) + 1;
 
-	if (strlen(root_path) > offset)
-		seq_show_option(seq, root_path + offset, NULL);
+	if (strlen(sb_data->host_root_path) > offset)
+		seq_show_option(seq, sb_data->host_root_path + offset, NULL);
 
 	if (append)
 		seq_puts(seq, ",append");
 
+	if (sb_data->use_xattr == 0)
+		seq_puts(seq, ",noxattrperm");
+
+	if (sb_data->use_xattr == 1)
+		seq_puts(seq, ",xattrperm");
+
 	return 0;
 }
 
@@ -516,7 +536,8 @@ static int read_name(struct inode *ino, char *name)
 {
 	dev_t rdev;
 	struct hostfs_stat st;
-	int err = stat_file(name, &st, -1);
+	struct hostfs_fs_info *sb_data = ino->i_sb->s_fs_info;
+	int err = stat_file(name, &st, -1, sb_data->use_xattr);
 	if (err)
 		return err;
 
@@ -564,9 +585,13 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
 			 struct dentry *dentry, umode_t mode, bool excl)
 {
 	struct inode *inode;
+	struct hostfs_fs_info *sb_data;
 	char *name;
 	int error, fd;
+	unsigned int currentuid;
+	unsigned int currentgid;
 
+	sb_data = dentry->d_sb->s_fs_info;
 	inode = hostfs_iget(dir->i_sb);
 	if (IS_ERR(inode)) {
 		error = PTR_ERR(inode);
@@ -578,7 +603,10 @@ static int hostfs_create(struct mnt_idmap *idmap, struct inode *dir,
 	if (name == NULL)
 		goto out_put;
 
-	fd = file_create(name, mode & 0777);
+	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
+	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
+	fd = file_create(name, mode & 0777, currentuid, currentgid,
+							   sb_data->use_xattr);
 	if (fd < 0)
 		error = fd;
 	else
@@ -675,12 +703,18 @@ static int hostfs_symlink(struct mnt_idmap *idmap, struct inode *ino,
 static int hostfs_mkdir(struct mnt_idmap *idmap, struct inode *ino,
 			struct dentry *dentry, umode_t mode)
 {
+	struct hostfs_fs_info *sb_data;
 	char *file;
 	int err;
+	unsigned int currentuid;
+	unsigned int currentgid;
 
+	sb_data = dentry->d_sb->s_fs_info;
 	if ((file = dentry_name(dentry)) == NULL)
 		return -ENOMEM;
-	err = do_mkdir(file, mode);
+	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
+	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);
+	err = do_mkdir(file, mode, currentuid, currentgid, sb_data->use_xattr);
 	__putname(file);
 	return err;
 }
@@ -796,10 +830,12 @@ static int hostfs_setattr(struct mnt_idmap *idmap,
 {
 	struct inode *inode = d_inode(dentry);
 	struct hostfs_iattr attrs;
+	struct hostfs_fs_info *sb_data;
 	char *name;
 	int err;
 
 	int fd = HOSTFS_I(inode)->fd;
+	sb_data = dentry->d_sb->s_fs_info;
 
 	err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
 	if (err)
@@ -849,7 +885,7 @@ static int hostfs_setattr(struct mnt_idmap *idmap,
 	name = dentry_name(dentry);
 	if (name == NULL)
 		return -ENOMEM;
-	err = set_attr(name, &attrs, fd);
+	err = set_attr(name, &attrs, fd, sb_data->use_xattr);
 	__putname(name);
 	if (err)
 		return err;
@@ -915,10 +951,58 @@ static const struct inode_operations hostfs_link_iops = {
 	.get_link	= hostfs_get_link,
 };
 
+static int hostfs_parse_options(struct hostfs_fs_info *sb_data, const char *d)
+{
+	int err = 0, unknown = 0;
+	char *ptr, *options, *string;
+
+	snprintf(sb_data->host_root_path, PATH_MAX, "%s/", root_ino);
+	sb_data->use_xattr = -1;
+	if (d == NULL)
+		return 0;
+
+	string = kmalloc(strlen(d) + 1, GFP_KERNEL);
+	if (string == NULL) {
+		err = -ENOMEM;
+		goto out;
+	}
+	options = string;
+	strcpy(options, d);
+
+	while (options) {
+		ptr = strchr(options, ',');
+		if (ptr != NULL)
+			*ptr++ = '\0';
+		if (*options != '\0') {
+			if (!strcmp(options, "noxattrperm")) {
+				sb_data->use_xattr = 0;
+				goto _break;
+			}
+			if (!strcmp(options, "xattrperm")) {
+				sb_data->use_xattr = 1;
+				goto _break;
+			}
+			if (!unknown) {
+				unknown = 1;
+				strcat(sb_data->host_root_path, options);
+				goto _break;
+			}
+			pr_warn("hostfs: unsupported mount option\n");
+			err = -EINVAL;
+			goto out;
+		}
+_break:
+		options = ptr;
+	}
+
+out:
+	kfree(string);
+	return err;
+}
 static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 {
 	struct inode *root_inode;
-	char *host_root_path, *req_root = d;
+	struct hostfs_fs_info *sb_data;
 	int err;
 
 	sb->s_blocksize = 1024;
@@ -931,26 +1015,26 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 	if (err)
 		goto out;
 
-	/* NULL is printed as '(null)' by printf(): avoid that. */
-	if (req_root == NULL)
-		req_root = "";
-
 	err = -ENOMEM;
-	sb->s_fs_info = host_root_path =
-		kasprintf(GFP_KERNEL, "%s/%s", root_ino, req_root);
-	if (host_root_path == NULL)
+	sb_data = kmalloc(sizeof(struct hostfs_fs_info), GFP_KERNEL);
+	if (sb_data == NULL)
+		goto out;
+
+	err = hostfs_parse_options(sb_data, d);
+	if (err != 0)
 		goto out;
+	sb->s_fs_info = sb_data;
 
 	root_inode = new_inode(sb);
 	if (!root_inode)
 		goto out;
 
-	err = read_name(root_inode, host_root_path);
+	err = read_name(root_inode, sb_data->host_root_path);
 	if (err)
 		goto out_put;
 
 	if (S_ISLNK(root_inode->i_mode)) {
-		char *name = follow_link(host_root_path);
+		char *name = follow_link(sb_data->host_root_path);
 		if (IS_ERR(name)) {
 			err = PTR_ERR(name);
 			goto out_put;
@@ -1001,6 +1085,9 @@ static int __init init_hostfs(void)
 	hostfs_inode_cache = KMEM_CACHE(hostfs_inode_info, 0);
 	if (!hostfs_inode_cache)
 		return -ENOMEM;
+	#ifdef MODULE
+	use_xattr = 0;
+	#endif
 	return register_filesystem(&hostfs_type);
 }
 
diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
index 5ecc4706172b..f5dd524cd153 100644
--- a/fs/hostfs/hostfs_user.c
+++ b/fs/hostfs/hostfs_user.c
@@ -10,14 +10,19 @@
 #include <errno.h>
 #include <fcntl.h>
 #include <string.h>
+#include <stdlib.h>
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/types.h>
 #include <sys/vfs.h>
 #include <sys/syscall.h>
+#include <sys/xattr.h>
+#include <um_malloc.h>
 #include "hostfs.h"
 #include <utime.h>
 
+int use_xattr;
+
 static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
 {
 	p->ino = buf->st_ino;
@@ -38,7 +43,184 @@ static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
 	p->min = os_minor(buf->st_rdev);
 }
 
-int stat_file(const char *path, struct hostfs_stat *p, int fd)
+static int uml_chown(const char *pathname, uid_t owner, gid_t group,
+							int mnt_use_xattr)
+{
+	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
+	/* 10 digits uid, 10 digits gid, null byte and ',' */
+	char umlcred[22];
+	/* max_gid=4294967295 - 10 digits + null byte */
+	char gid[11];
+	int i = 0;
+
+	if (xattr) {
+		memset(umlcred, 0, sizeof(umlcred));
+		getxattr(pathname, "user.umlcred", umlcred, sizeof(umlcred));
+
+		while (umlcred[i] != ',' && umlcred[i] != '\0')
+			i++;
+		umlcred[i] = '\0';
+
+		if (group == -1)
+			strncpy(gid, &umlcred[i+1], sizeof(gid));
+		else
+			snprintf(gid, sizeof(gid), "%u", group);
+		if (owner != -1)
+			snprintf(umlcred, sizeof(gid), "%u", owner);
+
+		strcat(umlcred, ",");
+		strncat(umlcred, gid, sizeof(gid-1));
+		if (setxattr(pathname, "user.umlcred", umlcred,
+						strlen(umlcred)+1, 0))
+			return -errno;
+
+		return 0;
+	}
+	if (chown(pathname, owner, group))
+		return -errno;
+	return 0;
+}
+
+static int uml_fchown(int fd, uid_t owner, gid_t group, int mnt_use_xattr)
+{
+	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
+	/* 10 digits uid, 10 digits gid, null byte and ',' */
+	char umlcred[22];
+	/* max_gid=4294967295 - 10 digits + null byte */
+	char gid[11];
+	int i = 0;
+
+	if (xattr) {
+		memset(umlcred, 0, sizeof(umlcred));
+		fgetxattr(fd, "user.umlcred", umlcred, sizeof(umlcred));
+
+		while (umlcred[i] != ',' && umlcred[i] != '\0')
+			i++;
+		umlcred[i] = '\0';
+
+		if (group == -1)
+			strncpy(gid, &umlcred[i+1], sizeof(gid));
+		else
+			snprintf(gid, sizeof(gid), "%u", group);
+		if (owner != -1)
+			snprintf(umlcred, sizeof(gid), "%u", owner);
+
+		strcat(umlcred, ",");
+		strncat(umlcred, gid, sizeof(gid-1));
+		if (fsetxattr(fd, "user.umlcred", umlcred, strlen(umlcred)+1, 0))
+			return -errno;
+
+		return 0;
+	}
+	if (fchown(fd, owner, group))
+		return -errno;
+	return 0;
+}
+
+static int uml_chmod(const char *pathname, unsigned int mode, int mnt_use_xattr)
+{
+	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
+	int i;
+	char mode_string[7];	/* mode is 16-bit -> max 6 digits in octal */
+
+	if (xattr) {
+		strcpy(mode_string, "000000");
+		i = sizeof(mode_string) - 1;
+		while (mode > 0) {
+			mode_string[i] = (mode % 8) + '0';
+			mode = mode / 8;
+			i--;
+		}
+		if (setxattr(pathname, "user.umlmode", mode_string,
+						       sizeof(mode_string), 0))
+			return -errno;
+		return 0;
+	}
+	if (chmod(pathname, mode))
+		return -errno;
+	return 0;
+}
+
+static int uml_fchmod(int fd, unsigned int mode, int mnt_use_xattr)
+{
+	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
+	int i;
+	char mode_string[7];	/* mode is 16-bit -> max 6 digits in octal */
+
+	if (xattr) {
+		strcpy(mode_string, "000000");
+		i = sizeof(mode_string) - 1;
+		while (mode > 0) {
+			mode_string[i] = (mode % 8) + '0';
+			mode = mode / 8;
+			i--;
+		}
+		if (fsetxattr(fd, "user.umlmode", mode_string,
+						  sizeof(mode_string), 0))
+			return -errno;
+		return 0;
+	}
+	if (fchmod(fd, mode))
+		return -errno;
+	return 0;
+}
+
+static void read_permissions(const char *path, struct stat64 *p,
+							int mnt_use_xattr)
+{
+	unsigned int mode = 0, i;
+	char mode_string[7], umlcred[22];
+	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
+
+	if (!xattr)
+		return;
+
+	if (getxattr(path, "user.umlmode", mode_string, sizeof(mode_string)) != -1) {
+		i = 0;
+		while (mode_string[i] != '\0') {
+			mode *= 8;
+			mode += mode_string[i] - '0';
+			i++;
+		}
+		p->st_mode = mode;
+	}
+
+	if (getxattr(path, "user.umlcred", umlcred, sizeof(umlcred)) != -1) {
+		i = 0;
+		while (umlcred[i] != ',')
+			i++;
+		umlcred[i] = '\0';
+
+		if (strlen(umlcred) > 0)
+			p->st_uid = atoi(umlcred);
+		if (strlen(&umlcred[i+1]) > 0)
+			p->st_gid = atoi(&umlcred[i+1]);
+	}
+}
+
+static long is_set_gid(const char *path, int mnt_use_xattr)
+{
+	int i = strlen(path) + 1;
+	char *parent = uml_kmalloc(i, UM_GFP_KERNEL);
+	struct stat64 buf = { 0 };
+
+	if (parent == NULL)
+		return -1;
+	strcpy(parent, path);
+	i = i - 3;
+	while (parent[i] != '/')
+		i--;
+	parent[i] = '\0';
+
+	stat64(parent, &buf);
+	read_permissions(parent, &buf, mnt_use_xattr);
+	kfree(parent);
+	if (buf.st_mode & S_ISGID)
+		return buf.st_gid;
+	return -1;
+}
+
+int stat_file(const char *path, struct hostfs_stat *p, int fd, int mnt_use_xattr)
 {
 	struct stat64 buf;
 
@@ -48,6 +230,7 @@ int stat_file(const char *path, struct hostfs_stat *p, int fd)
 	} else if (lstat64(path, &buf) < 0) {
 		return -errno;
 	}
+	read_permissions(path, &buf, mnt_use_xattr);
 	stat64_to_hostfs(&buf, p);
 	return 0;
 }
@@ -181,44 +364,60 @@ void close_dir(void *stream)
 	closedir(stream);
 }
 
-int file_create(char *name, int mode)
+int file_create(char *name, int mode, uid_t uid, gid_t gid, int mnt_use_xattr)
 {
 	int fd;
+	long ret;
 
 	fd = open64(name, O_CREAT | O_RDWR, mode);
 	if (fd < 0)
 		return -errno;
+
+	ret = is_set_gid(name, mnt_use_xattr);
+	if (ret != -1)
+		gid = ret;
+	uml_chown(name, uid, gid, mnt_use_xattr);
 	return fd;
 }
 
-int set_attr(const char *file, struct hostfs_iattr *attrs, int fd)
+int set_attr(const char *file, struct hostfs_iattr *attrs, int fd,
+							     int mnt_use_xattr)
 {
 	struct hostfs_stat st;
 	struct timeval times[2];
-	int err, ma;
+	int err, ma, ret;
 
 	if (attrs->ia_valid & HOSTFS_ATTR_MODE) {
 		if (fd >= 0) {
-			if (fchmod(fd, attrs->ia_mode) != 0)
-				return -errno;
-		} else if (chmod(file, attrs->ia_mode) != 0) {
-			return -errno;
+			ret = uml_fchmod(fd, attrs->ia_mode, mnt_use_xattr);
+			if (ret != 0)
+				return ret;
+		} else {
+			ret = uml_chmod(file, attrs->ia_mode, mnt_use_xattr);
+			if (ret != 0)
+				return ret;
 		}
 	}
 	if (attrs->ia_valid & HOSTFS_ATTR_UID) {
 		if (fd >= 0) {
-			if (fchown(fd, attrs->ia_uid, -1))
-				return -errno;
-		} else if (chown(file, attrs->ia_uid, -1)) {
-			return -errno;
+			ret = uml_fchown(fd, attrs->ia_uid, -1, mnt_use_xattr);
+			if (ret != 0)
+				return ret;
+		} else {
+			ret = uml_chown(file, attrs->ia_uid, -1, mnt_use_xattr);
+			if (ret != 0)
+				return ret;
 		}
 	}
 	if (attrs->ia_valid & HOSTFS_ATTR_GID) {
 		if (fd >= 0) {
-			if (fchown(fd, -1, attrs->ia_gid))
-				return -errno;
-		} else if (chown(file, -1, attrs->ia_gid)) {
-			return -errno;
+			ret = uml_fchown(fd, -1, attrs->ia_gid, mnt_use_xattr);
+			if (ret != 0)
+				return ret;
+		} else {
+			ret = uml_chown(file, -1, attrs->ia_gid, mnt_use_xattr);
+			if (ret != 0)
+				return ret;
 		}
 	}
 	if (attrs->ia_valid & HOSTFS_ATTR_SIZE) {
@@ -237,7 +436,7 @@ int set_attr(const char *file, struct hostfs_iattr *attrs, int fd)
 	 */
 	ma = (HOSTFS_ATTR_ATIME_SET | HOSTFS_ATTR_MTIME_SET);
 	if (attrs->ia_valid & ma) {
-		err = stat_file(file, &st, fd);
+		err = stat_file(file, &st, fd, mnt_use_xattr);
 		if (err != 0)
 			return err;
 
@@ -265,7 +464,7 @@ int set_attr(const char *file, struct hostfs_iattr *attrs, int fd)
 
 	/* Note: ctime is not handled */
 	if (attrs->ia_valid & (HOSTFS_ATTR_ATIME | HOSTFS_ATTR_MTIME)) {
-		err = stat_file(file, &st, fd);
+		err = stat_file(file, &st, fd, mnt_use_xattr);
 		attrs->ia_atime = st.atime;
 		attrs->ia_mtime = st.mtime;
 		if (err != 0)
@@ -294,13 +493,18 @@ int unlink_file(const char *file)
 	return 0;
 }
 
-int do_mkdir(const char *file, int mode)
+int do_mkdir(const char *file, int mode, uid_t uid, gid_t gid, int mnt_use_xattr)
 {
 	int err;
+	long ret;
 
 	err = mkdir(file, mode);
 	if (err)
 		return -errno;
+	ret = is_set_gid(file, mnt_use_xattr);
+	if (ret != -1)
+		gid = ret;
+	uml_chown(file, uid, gid, mnt_use_xattr);
 	return 0;
 }
 
-- 
2.39.2


_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v3 2/2] hostfs: store permissions in extended attributes

[Marko Petrović]

Hello,

I have written the third version of the patch. Thank you for all of your
recommendations.

While writing the third patch version, I noticed that there was a
serious limitation of the code in second patch, namely the whole
xattrperm feature was available only and only as boot time flag so it
could not be used when hostfs was built as module since modules don't
have hostfs_args() function.

To overcome that issue, I have changed the content of
struct super_block -> s_fs_info to point to a struct hostfs_fs_info
containing the string that was previously there (to be used by old
functions) and the per-mountpoint use_xattr flag.
This allows easy extending of mount options in the future and thus
providing more flexibility to userspace to configure the filesystem.
For example, hostfs_attr, acl and noacl could be the mount options added
for POSIX ACLs and extended attributes in the future and if there is a
desire for that, append could become a mount option now too.

Regarding xattrperm as the kernel boot parameter, I left it available and
it defines the default behavior when mounting the filesystem (when
neither xattrperm nor noxattrperm is specified in mount options).

What do you think about this new change? Please, let me know if you have
any concerns so that I can address them.

Best regards,
Marko Petrović

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v3 2/2] hostfs: store permissions in extended attributes

[Johannes Berg]

Hi,

On Sun, 2023-04-16 at 19:24 +0200, Marko Petrović wrote:
> 
> I have written the third version of the patch. Thank you for all of your
> recommendations.
> 
> While writing the third patch version, I noticed that there was a
> serious limitation of the code in second patch, namely the whole
> xattrperm feature was available only and only as boot time flag so it
> could not be used when hostfs was built as module since modules don't
> have hostfs_args() function.
> 
> To overcome that issue, I have changed the content of
> struct super_block -> s_fs_info to point to a struct hostfs_fs_info
> containing the string that was previously there (to be used by old
> functions) and the per-mountpoint use_xattr flag.
> This allows easy extending of mount options in the future and thus
> providing more flexibility to userspace to configure the filesystem.
> For example, hostfs_attr, acl and noacl could be the mount options added
> for POSIX ACLs and extended attributes in the future and if there is a
> desire for that, append could become a mount option now too.
> 
> Regarding xattrperm as the kernel boot parameter, I left it available and
> it defines the default behavior when mounting the filesystem (when
> neither xattrperm nor noxattrperm is specified in mount options).

It seems that _maybe_, similar to the 'hostfs' kernel argument, there
should be a way to contain the set of options?

What do I mean by that? I mean that today, the inside of the virtual
machine (for lack of a better word) can only mount outside folders that
are contained in the folder indicated by the 'hostfs' argument.
Similarly, perhaps the "outside administrator" should be able to
indicate that xattr permissions _must_ be used, or _must not_ be used?

Which would imply a new kernel argument that can be configured to "force
use", "force don't use" and "don't care", with perhaps even for backward
compatibility the default being "force don't use"?

Not sure. Anton? Richard? Any opinions?

johannes

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v3 2/2] hostfs: store permissions in extended attributes

[Marko Petrović]

On Tue Apr 18, 2023 at 10:31 AM CEST, Johannes Berg wrote:
Hello,
Thanks for your reply!
> Hi,
>
> On Sun, 2023-04-16 at 19:24 +0200, Marko Petrović wrote:
> > 
> > I have written the third version of the patch. Thank you for all of your
> > recommendations.
> > 
> > While writing the third patch version, I noticed that there was a
> > serious limitation of the code in second patch, namely the whole
> > xattrperm feature was available only and only as boot time flag so it
> > could not be used when hostfs was built as module since modules don't
> > have hostfs_args() function.
> > 
> > To overcome that issue, I have changed the content of
> > struct super_block -> s_fs_info to point to a struct hostfs_fs_info
> > containing the string that was previously there (to be used by old
> > functions) and the per-mountpoint use_xattr flag.
> > This allows easy extending of mount options in the future and thus
> > providing more flexibility to userspace to configure the filesystem.
> > For example, hostfs_attr, acl and noacl could be the mount options added
> > for POSIX ACLs and extended attributes in the future and if there is a
> > desire for that, append could become a mount option now too.
> > 
> > Regarding xattrperm as the kernel boot parameter, I left it available and
> > it defines the default behavior when mounting the filesystem (when
> > neither xattrperm nor noxattrperm is specified in mount options).
>
> It seems that _maybe_, similar to the 'hostfs' kernel argument, there
> should be a way to contain the set of options?
>
> What do I mean by that? I mean that today, the inside of the virtual
> machine (for lack of a better word) can only mount outside folders that
> are contained in the folder indicated by the 'hostfs' argument.
> Similarly, perhaps the "outside administrator" should be able to
> indicate that xattr permissions _must_ be used, or _must not_ be used?
Nice observation. It shouldn't be hard to do this, I can just change
the interpreted meaning of mnt_use_xattr and hostfs_fs_info->use_xattr
to comply with this behavior. Thanks for bringing this to attention.
>
> Which would imply a new kernel argument that can be configured to "force
> use", "force don't use" and "don't care", with perhaps even for backward
> compatibility the default being "force don't use"?
>
> Not sure. Anton? Richard? Any opinions?
Maybe xattrperm and noxattrperm can be kernel command line arguments
used for "force use" and "force don't use" and when none is specified,
the behavior could be "don't care" which would thus be the default.

That may also be reasonable for the purpose of backward compatibility
since the usage of extended attributes would then be specified as an mount
option and applications not aware of it would just use the old behavior
(since the extended attributes would be used only when specified in
mount options).

On the other hand, that would require a little different mounting of
root filesystem. Maybe adding rootxattrperm as a new kernel command line
argument for mounting root with "rootfstype=hostfs hostfs=rootxattrperm"
could be the solution (for when root should use extended attributes, but
the general behavior should still be "don't care")?
What are your opinions?

Best regards,
Marko Petrović

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v3 2/2] hostfs: store permissions in extended attributes

[Johannes Berg]

On Tue, 2023-04-25 at 18:35 +0200, Marko Petrović wrote:
> > It seems that _maybe_, similar to the 'hostfs' kernel argument, there
> > should be a way to contain the set of options?
> > 
> > What do I mean by that? I mean that today, the inside of the virtual
> > machine (for lack of a better word) can only mount outside folders that
> > are contained in the folder indicated by the 'hostfs' argument.
> > Similarly, perhaps the "outside administrator" should be able to
> > indicate that xattr permissions _must_ be used, or _must not_ be used?
> Nice observation. It shouldn't be hard to do this, I can just change
> the interpreted meaning of mnt_use_xattr and hostfs_fs_info->use_xattr
> to comply with this behavior. Thanks for bringing this to attention.
> > 
> > Which would imply a new kernel argument that can be configured to "force
> > use", "force don't use" and "don't care", with perhaps even for backward
> > compatibility the default being "force don't use"?
> > 
> > Not sure. Anton? Richard? Any opinions?

> Maybe xattrperm and noxattrperm can be kernel command line arguments
> used for "force use" and "force don't use" and when none is specified,
> the behavior could be "don't care" which would thus be the default.

Right. Actually now looking at this again, they should probably be flags
inside the hostfs= argument? Like the "append" flag now.

Not really sure what the default should be, perhaps it makes sense to
not allow it by default so it's the same as now? But I don't know how
strict we need to be about this.

> That may also be reasonable for the purpose of backward compatibility
> since the usage of extended attributes would then be specified as an mount
> option and applications not aware of it would just use the old behavior
> (since the extended attributes would be used only when specified in
> mount options).

Right. I was more thinking of the isolation aspects of this.

> On the other hand, that would require a little different mounting of
> root filesystem. Maybe adding rootxattrperm as a new kernel command line
> argument for mounting root with "rootfstype=hostfs hostfs=rootxattrperm"
> could be the solution (for when root should use extended attributes, but
> the general behavior should still be "don't care")?
> What are your opinions?

Oh, that's a good point too. I don't think I have much of an opinion on
it though. But yeah, why not have another flag "rootxattrperm" for the
hostfs= option, along with xattrperm and noxattrperm (or allowxattrperm
and forcexattrperm if we need noxattrperm to be the default per above.)

johannes

PS: Note that in uml/next my patches with the split are merged, so when
you rebase please rebase on that and adjust accordingly with the
exported symbol we discussed.

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um

Re: [PATCH v3 2/2] hostfs: store permissions in extended attributes

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "Marko Petrović" <petrovicmarko2006@gmail.com>
> An: "linux-um" <linux-um@lists.infradead.org>
> CC: "richard" <richard@nod.at>, "anton ivanov" <anton.ivanov@cambridgegreys.com>, "Johannes Berg"
> <johannes@sipsolutions.net>, "Marko Petrović" <petrovicmarko2006@gmail.com>
> Gesendet: Samstag, 15. April 2023 18:48:21
> Betreff: [PATCH v3 2/2] hostfs: store permissions in extended attributes

> This patch adds support for xattrperm hostfs kernel command line option
> which enables the use of extended attributes for storing permissions by
> default on all mounted hostfs filesystems.

Sorry for the delay. Just unearthed this patch from patchwork.
 
> The support for per-mountpoint xattrperm/noxattrperm is added.
> 
> 'struct super_block -> s_fs_info' now points to 'struct hostfs_fs_info'.
> All code that relied on s_fs_info pointing to a string is changed to use
> the same string stored inside 'struct hostfs_fs_info'. This allows easy
> extensions of the super_block data in the future for storing mount
> options.
> 
> Function hostfs_parse_options() is added for parsing the string of
> mount options and storing them in 'struct hostfs_fs_info'.
> 
> When xattrperm is enabled, filesystem stores permissions in a
> human-readable string in attributes user.umlmode (for mode) and
> user.umlcred (for uid and gid).
> 
> Signed-off-by: Marko Petrović <petrovicmarko2006@gmail.com>
> ---
> fs/hostfs/hostfs.h      |  13 ++-
> fs/hostfs/hostfs_kern.c | 129 +++++++++++++++++----
> fs/hostfs/hostfs_user.c | 242 ++++++++++++++++++++++++++++++++++++----
> 3 files changed, 340 insertions(+), 44 deletions(-)
> 
> diff --git a/fs/hostfs/hostfs.h b/fs/hostfs/hostfs.h
> index 69cb796f6270..03f773b7c423 100644
> --- a/fs/hostfs/hostfs.h
> +++ b/fs/hostfs/hostfs.h
> @@ -37,6 +37,7 @@
>  * is on, and remove the appropriate bits from attr->ia_mode (attr is a
>  * "struct iattr *"). -BlaisorBlade
>  */
> +extern int use_xattr;
> struct hostfs_timespec {
> 	long long tv_sec;
> 	long long tv_nsec;
> @@ -67,7 +68,8 @@ struct hostfs_stat {
> 	unsigned int min;
> };
> 
> -extern int stat_file(const char *path, struct hostfs_stat *p, int fd);
> +extern int stat_file(const char *path, struct hostfs_stat *p, int fd,
> +							int mnt_use_xattr);
> extern int access_file(char *path, int r, int w, int x);
> extern int open_file(char *path, int r, int w, int append);
> extern void *open_dir(char *path, int *err_out);
> @@ -83,11 +85,14 @@ extern int write_file(int fd, unsigned long long *offset,
> const char *buf,
> 		      int len);
> extern int lseek_file(int fd, long long offset, int whence);
> extern int fsync_file(int fd, int datasync);
> -extern int file_create(char *name, int mode);
> -extern int set_attr(const char *file, struct hostfs_iattr *attrs, int fd);
> +extern int file_create(char *name, int mode, uid_t uid, gid_t gid,
> +							int mnt_use_xattr);
> +extern int set_attr(const char *file, struct hostfs_iattr *attrs, int fd,
> +							    int mnt_use_xattr);
> extern int make_symlink(const char *from, const char *to);
> extern int unlink_file(const char *file);
> -extern int do_mkdir(const char *file, int mode);
> +extern int do_mkdir(const char *file, int mode, uid_t uid, gid_t gid,
> +							int mnt_use_xattr);
> extern int hostfs_do_rmdir(const char *file);
> extern int do_mknod(const char *file, int mode, unsigned int major,
> 		    unsigned int minor);
> diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
> index 28b4f15c19eb..a2afe70abf14 100644
> --- a/fs/hostfs/hostfs_kern.c
> +++ b/fs/hostfs/hostfs_kern.c
> @@ -17,6 +17,7 @@
> #include <linux/writeback.h>
> #include <linux/mount.h>
> #include <linux/namei.h>
> +#include <linux/uidgid.h>
> #include "hostfs.h"
> #include <init.h>
> #include <kern.h>
> @@ -28,6 +29,11 @@ struct hostfs_inode_info {
> 	struct mutex open_mutex;
> };
> 
> +struct hostfs_fs_info {
> +	char host_root_path[PATH_MAX+1];

Why +1? AFAIK PATH_MAX includes space for the NUL byte.

> +	int use_xattr;

This is a very generic name.
Feel also free to use bool instead of int.

> +};
> +
> static inline struct hostfs_inode_info *HOSTFS_I(struct inode *inode)
> {
> 	return list_entry(inode, struct hostfs_inode_info, vfs_inode);
> @@ -64,6 +70,8 @@ static int __init hostfs_args(char *options, int *add)
> 		if (*options != '\0') {
> 			if (!strcmp(options, "append"))
> 				append = 1;
> +			else if (!strcmp(options, "xattrperm"))
> +				use_xattr = 1;
> 			else printf("hostfs_args - unsupported option - %s\n",
> 				    options);
> 		}
> @@ -79,18 +87,22 @@ __uml_setup("hostfs=", hostfs_args,
> "    tree on the host.  If this isn't specified, then a user inside UML can\n"
> "    mount anything on the host that's accessible to the user that's running\n"
> "    it.\n"
> -"    The only flag currently supported is 'append', which specifies that all\n"
> -"    files opened by hostfs will be opened in append mode.\n\n"
> +"    The only flags currently supported are 'append', which specifies that\n"
> +"    all files opened by hostfs will be opened in append mode and
> 'xattrperm'\n"
> +"    which specifies that permissions of files will be stored in extended\n"
> +"    attributes.\n\n"

Please mention that they are stored on xattrs on the host side.

> );
> #endif
> 
> static char *__dentry_name(struct dentry *dentry, char *name)
> {
> 	char *p = dentry_path_raw(dentry, name, PATH_MAX);
> +	struct hostfs_fs_info *sb_data;
> 	char *root;
> 	size_t len;
> 
> -	root = dentry->d_sb->s_fs_info;
> +	sb_data = dentry->d_sb->s_fs_info;
> +	root = sb_data->host_root_path;
> 	len = strlen(root);
> 	if (IS_ERR(p)) {
> 		__putname(name);
> @@ -203,8 +215,10 @@ static int hostfs_statfs(struct dentry *dentry, struct
> kstatfs *sf)
> 	long long f_bavail;
> 	long long f_files;
> 	long long f_ffree;
> +	struct hostfs_fs_info *sb_data;
> 
> -	err = do_statfs(dentry->d_sb->s_fs_info,
> +	sb_data = dentry->d_sb->s_fs_info;
> +	err = do_statfs(sb_data->host_root_path,
> 			&sf->f_bsize, &f_blocks, &f_bfree, &f_bavail, &f_files,
> 			&f_ffree, &sf->f_fsid, sizeof(sf->f_fsid),
> 			&sf->f_namelen);
> @@ -250,15 +264,21 @@ static void hostfs_free_inode(struct inode *inode)
> 
> static int hostfs_show_options(struct seq_file *seq, struct dentry *root)
> {
> -	const char *root_path = root->d_sb->s_fs_info;
> +	struct hostfs_fs_info *sb_data = root->d_sb->s_fs_info;
> 	size_t offset = strlen(root_ino) + 1;
> 
> -	if (strlen(root_path) > offset)
> -		seq_show_option(seq, root_path + offset, NULL);
> +	if (strlen(sb_data->host_root_path) > offset)
> +		seq_show_option(seq, sb_data->host_root_path + offset, NULL);
> 
> 	if (append)
> 		seq_puts(seq, ",append");
> 
> +	if (sb_data->use_xattr == 0)
> +		seq_puts(seq, ",noxattrperm");
> +
> +	if (sb_data->use_xattr == 1)
> +		seq_puts(seq, ",xattrperm");
> +
> 	return 0;
> }
> 
> @@ -516,7 +536,8 @@ static int read_name(struct inode *ino, char *name)
> {
> 	dev_t rdev;
> 	struct hostfs_stat st;
> -	int err = stat_file(name, &st, -1);
> +	struct hostfs_fs_info *sb_data = ino->i_sb->s_fs_info;
> +	int err = stat_file(name, &st, -1, sb_data->use_xattr);
> 	if (err)
> 		return err;
> 
> @@ -564,9 +585,13 @@ static int hostfs_create(struct mnt_idmap *idmap, struct
> inode *dir,
> 			 struct dentry *dentry, umode_t mode, bool excl)
> {
> 	struct inode *inode;
> +	struct hostfs_fs_info *sb_data;
> 	char *name;
> 	int error, fd;
> +	unsigned int currentuid;
> +	unsigned int currentgid;
> 
> +	sb_data = dentry->d_sb->s_fs_info;
> 	inode = hostfs_iget(dir->i_sb);
> 	if (IS_ERR(inode)) {
> 		error = PTR_ERR(inode);
> @@ -578,7 +603,10 @@ static int hostfs_create(struct mnt_idmap *idmap, struct
> inode *dir,
> 	if (name == NULL)
> 		goto out_put;
> 
> -	fd = file_create(name, mode & 0777);
> +	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
> +	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);

I think we should better use filesystem instead of the effective id.
These days they're in most cases the same, but applications could still use setfsuid() and friends.

> +	fd = file_create(name, mode & 0777, currentuid, currentgid,
> +							   sb_data->use_xattr);
> 	if (fd < 0)
> 		error = fd;
> 	else
> @@ -675,12 +703,18 @@ static int hostfs_symlink(struct mnt_idmap *idmap, struct
> inode *ino,
> static int hostfs_mkdir(struct mnt_idmap *idmap, struct inode *ino,
> 			struct dentry *dentry, umode_t mode)
> {
> +	struct hostfs_fs_info *sb_data;
> 	char *file;
> 	int err;
> +	unsigned int currentuid;
> +	unsigned int currentgid;
> 
> +	sb_data = dentry->d_sb->s_fs_info;
> 	if ((file = dentry_name(dentry)) == NULL)
> 		return -ENOMEM;
> -	err = do_mkdir(file, mode);
> +	currentuid = from_kuid(current->cred->user_ns, current->cred->euid);
> +	currentgid = from_kgid(current->cred->user_ns, current->cred->egid);

Same.
Also make this a helper function, please.

> +	err = do_mkdir(file, mode, currentuid, currentgid, sb_data->use_xattr);
> 	__putname(file);
> 	return err;
> }
> @@ -796,10 +830,12 @@ static int hostfs_setattr(struct mnt_idmap *idmap,
> {
> 	struct inode *inode = d_inode(dentry);
> 	struct hostfs_iattr attrs;
> +	struct hostfs_fs_info *sb_data;
> 	char *name;
> 	int err;
> 
> 	int fd = HOSTFS_I(inode)->fd;
> +	sb_data = dentry->d_sb->s_fs_info;
> 
> 	err = setattr_prepare(&nop_mnt_idmap, dentry, attr);
> 	if (err)
> @@ -849,7 +885,7 @@ static int hostfs_setattr(struct mnt_idmap *idmap,
> 	name = dentry_name(dentry);
> 	if (name == NULL)
> 		return -ENOMEM;
> -	err = set_attr(name, &attrs, fd);
> +	err = set_attr(name, &attrs, fd, sb_data->use_xattr);

Instead of passing use_xattr to very function just pass sb_data itself
as first parameter. Maybe it is useful later for passing other filesystem
context stuff.


> 	__putname(name);
> 	if (err)
> 		return err;
> @@ -915,10 +951,58 @@ static const struct inode_operations hostfs_link_iops = {
> 	.get_link	= hostfs_get_link,
> };
> 
> +static int hostfs_parse_options(struct hostfs_fs_info *sb_data, const char *d)
> +{
> +	int err = 0, unknown = 0;
> +	char *ptr, *options, *string;
> +
> +	snprintf(sb_data->host_root_path, PATH_MAX, "%s/", root_ino);

Why is host_root_path a static array? Just use kasprintf() or such.

> +	sb_data->use_xattr = -1;
> +	if (d == NULL)
> +		return 0;
> +
> +	string = kmalloc(strlen(d) + 1, GFP_KERNEL);
> +	if (string == NULL) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	options = string;
> +	strcpy(options, d);

Why not strdup()?

> +
> +	while (options) {
> +		ptr = strchr(options, ',');
> +		if (ptr != NULL)
> +			*ptr++ = '\0';
> +		if (*options != '\0') {
> +			if (!strcmp(options, "noxattrperm")) {
> +				sb_data->use_xattr = 0;
> +				goto _break;
> +			}
> +			if (!strcmp(options, "xattrperm")) {
> +				sb_data->use_xattr = 1;
> +				goto _break;
> +			}
> +			if (!unknown) {
> +				unknown = 1;
> +				strcat(sb_data->host_root_path, options);
> +				goto _break;
> +			}
> +			pr_warn("hostfs: unsupported mount option\n");
> +			err = -EINVAL;
> +			goto out;
> +		}
> +_break:
> +		options = ptr;
> +	}
> +
> +out:
> +	kfree(string);
> +	return err;
> +}
> static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
> {
> 	struct inode *root_inode;
> -	char *host_root_path, *req_root = d;
> +	struct hostfs_fs_info *sb_data;
> 	int err;
> 
> 	sb->s_blocksize = 1024;
> @@ -931,26 +1015,26 @@ static int hostfs_fill_sb_common(struct super_block *sb,
> void *d, int silent)
> 	if (err)
> 		goto out;
> 
> -	/* NULL is printed as '(null)' by printf(): avoid that. */
> -	if (req_root == NULL)
> -		req_root = "";
> -
> 	err = -ENOMEM;
> -	sb->s_fs_info = host_root_path =
> -		kasprintf(GFP_KERNEL, "%s/%s", root_ino, req_root);
> -	if (host_root_path == NULL)
> +	sb_data = kmalloc(sizeof(struct hostfs_fs_info), GFP_KERNEL);
> +	if (sb_data == NULL)
> +		goto out;
> +
> +	err = hostfs_parse_options(sb_data, d);
> +	if (err != 0)
> 		goto out;
> +	sb->s_fs_info = sb_data;
> 
> 	root_inode = new_inode(sb);
> 	if (!root_inode)
> 		goto out;
> 
> -	err = read_name(root_inode, host_root_path);
> +	err = read_name(root_inode, sb_data->host_root_path);

Also for such functions, just pass sb_data.

> 	if (err)
> 		goto out_put;
> 
> 	if (S_ISLNK(root_inode->i_mode)) {
> -		char *name = follow_link(host_root_path);
> +		char *name = follow_link(sb_data->host_root_path);
> 		if (IS_ERR(name)) {
> 			err = PTR_ERR(name);
> 			goto out_put;
> @@ -1001,6 +1085,9 @@ static int __init init_hostfs(void)
> 	hostfs_inode_cache = KMEM_CACHE(hostfs_inode_info, 0);
> 	if (!hostfs_inode_cache)
> 		return -ENOMEM;
> +	#ifdef MODULE
> +	use_xattr = 0;
> +	#endif

Why is this needed?

> 	return register_filesystem(&hostfs_type);
> }
> 
> diff --git a/fs/hostfs/hostfs_user.c b/fs/hostfs/hostfs_user.c
> index 5ecc4706172b..f5dd524cd153 100644
> --- a/fs/hostfs/hostfs_user.c
> +++ b/fs/hostfs/hostfs_user.c
> @@ -10,14 +10,19 @@
> #include <errno.h>
> #include <fcntl.h>
> #include <string.h>
> +#include <stdlib.h>
> #include <sys/stat.h>
> #include <sys/time.h>
> #include <sys/types.h>
> #include <sys/vfs.h>
> #include <sys/syscall.h>
> +#include <sys/xattr.h>
> +#include <um_malloc.h>
> #include "hostfs.h"
> #include <utime.h>
> 
> +int use_xattr;
> +
> static void stat64_to_hostfs(const struct stat64 *buf, struct hostfs_stat *p)
> {
> 	p->ino = buf->st_ino;
> @@ -38,7 +43,184 @@ static void stat64_to_hostfs(const struct stat64 *buf,
> struct hostfs_stat *p)
> 	p->min = os_minor(buf->st_rdev);
> }
> 
> -int stat_file(const char *path, struct hostfs_stat *p, int fd)
> +static int uml_chown(const char *pathname, uid_t owner, gid_t group,
> +							int mnt_use_xattr)
> +{
> +	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;

At this level you should not distinguish between a flag set at mount time or globally.
Use one variable and set it at mount time.
e.g. the global module flag is the default and a mount option can override it.

> +	/* 10 digits uid, 10 digits gid, null byte and ',' */
> +	char umlcred[22];
> +	/* max_gid=4294967295 - 10 digits + null byte */
> +	char gid[11];
> +	int i = 0;
> +
> +	if (xattr) {
> +		memset(umlcred, 0, sizeof(umlcred));
> +		getxattr(pathname, "user.umlcred", umlcred, sizeof(umlcred));

You need to check the return code. What if the xattr is not there?

> +		while (umlcred[i] != ',' && umlcred[i] != '\0')
> +			i++;

What if the xattr contains garbage and no NULL terminator?
...then you loop here forever.

> +		umlcred[i] = '\0';
> +
> +		if (group == -1)
> +			strncpy(gid, &umlcred[i+1], sizeof(gid));
> +		else
> +			snprintf(gid, sizeof(gid), "%u", group);
> +		if (owner != -1)
> +			snprintf(umlcred, sizeof(gid), "%u", owner);
> +
> +		strcat(umlcred, ",");
> +		strncat(umlcred, gid, sizeof(gid-1));

I'm really not a fan of such old school C string hackery.
Can't you read using ssscanf() and produce the new string with snprintf() or such?

> +		if (setxattr(pathname, "user.umlcred", umlcred,
> +						strlen(umlcred)+1, 0))
> +			return -errno;
> +
> +		return 0;
> +	}
> +	if (chown(pathname, owner, group))
> +		return -errno;
> +	return 0;
> +}
> +
> +static int uml_fchown(int fd, uid_t owner, gid_t group, int mnt_use_xattr)
> +{
> +	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
> +	/* 10 digits uid, 10 digits gid, null byte and ',' */
> +	char umlcred[22];
> +	/* max_gid=4294967295 - 10 digits + null byte */
> +	char gid[11];
> +	int i = 0;
> +
> +	if (xattr) {
> +		memset(umlcred, 0, sizeof(umlcred));
> +		fgetxattr(fd, "user.umlcred", umlcred, sizeof(umlcred));
> +
> +		while (umlcred[i] != ',' && umlcred[i] != '\0')
> +			i++;
> +		umlcred[i] = '\0';
> +
> +		if (group == -1)
> +			strncpy(gid, &umlcred[i+1], sizeof(gid));
> +		else
> +			snprintf(gid, sizeof(gid), "%u", group);
> +		if (owner != -1)
> +			snprintf(umlcred, sizeof(gid), "%u", owner);
> +
> +		strcat(umlcred, ",");
> +		strncat(umlcred, gid, sizeof(gid-1));

Please add a helper function for common code.

> +		if (fsetxattr(fd, "user.umlcred", umlcred, strlen(umlcred)+1, 0))
> +			return -errno;
> +
> +		return 0;
> +	}
> +	if (fchown(fd, owner, group))
> +		return -errno;
> +	return 0;
> +}
> +
> +static int uml_chmod(const char *pathname, unsigned int mode, int
> mnt_use_xattr)
> +{
> +	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
> +	int i;
> +	char mode_string[7];	/* mode is 16-bit -> max 6 digits in octal */
> +
> +	if (xattr) {
> +		strcpy(mode_string, "000000");
> +		i = sizeof(mode_string) - 1;
> +		while (mode > 0) {
> +			mode_string[i] = (mode % 8) + '0';
> +			mode = mode / 8;
> +			i--;
> +		}

kstrtoint()?

> +		if (setxattr(pathname, "user.umlmode", mode_string,
> +						       sizeof(mode_string), 0))

So we have user.umlcred and user.umlmode xattrs on the host side.
Why not just one xattr?

> +			return -errno;
> +		return 0;
> +	}
> +	if (chmod(pathname, mode))
> +		return -errno;
> +	return 0;
> +}
> +
> +static int uml_fchmod(int fd, unsigned int mode, int mnt_use_xattr)
> +{
> +	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
> +	int i;
> +	char mode_string[7];	/* mode is 16-bit -> max 6 digits in octal */
> +
> +	if (xattr) {
> +		strcpy(mode_string, "000000");
> +		i = sizeof(mode_string) - 1;
> +		while (mode > 0) {
> +			mode_string[i] = (mode % 8) + '0';
> +			mode = mode / 8;
> +			i--;
> +		}
> +		if (fsetxattr(fd, "user.umlmode", mode_string,
> +						  sizeof(mode_string), 0))
> +			return -errno;
> +		return 0;
> +	}
> +	if (fchmod(fd, mode))
> +		return -errno;
> +	return 0;
> +}
> +
> +static void read_permissions(const char *path, struct stat64 *p,
> +							int mnt_use_xattr)
> +{
> +	unsigned int mode = 0, i;
> +	char mode_string[7], umlcred[22];
> +	int xattr = (mnt_use_xattr == -1) ? use_xattr : mnt_use_xattr;
> +
> +	if (!xattr)
> +		return;
> +
> +	if (getxattr(path, "user.umlmode", mode_string, sizeof(mode_string)) != -1) {
> +		i = 0;
> +		while (mode_string[i] != '\0') {
> +			mode *= 8;
> +			mode += mode_string[i] - '0';
> +			i++;
> +		}
> +		p->st_mode = mode;
> +	}
> +
> +	if (getxattr(path, "user.umlcred", umlcred, sizeof(umlcred)) != -1) {
> +		i = 0;
> +		while (umlcred[i] != ',')
> +			i++;
> +		umlcred[i] = '\0';
> +
> +		if (strlen(umlcred) > 0)
> +			p->st_uid = atoi(umlcred);
> +		if (strlen(&umlcred[i+1]) > 0)
> +			p->st_gid = atoi(&umlcred[i+1]);
> +	}
> +}
> +
> +static long is_set_gid(const char *path, int mnt_use_xattr)
> +{
> +	int i = strlen(path) + 1;
> +	char *parent = uml_kmalloc(i, UM_GFP_KERNEL);
> +	struct stat64 buf = { 0 };
> +
> +	if (parent == NULL)
> +		return -1;
> +	strcpy(parent, path);
> +	i = i - 3;
> +	while (parent[i] != '/')
> +		i--;
> +	parent[i] = '\0';
> +
> +	stat64(parent, &buf);
> +	read_permissions(parent, &buf, mnt_use_xattr);
> +	kfree(parent);
> +	if (buf.st_mode & S_ISGID)
> +		return buf.st_gid;
> +	return -1;
> +}
> +
> +int stat_file(const char *path, struct hostfs_stat *p, int fd, int
> mnt_use_xattr)
> {
> 	struct stat64 buf;
> 
> @@ -48,6 +230,7 @@ int stat_file(const char *path, struct hostfs_stat *p, int
> fd)
> 	} else if (lstat64(path, &buf) < 0) {
> 		return -errno;
> 	}
> +	read_permissions(path, &buf, mnt_use_xattr);
> 	stat64_to_hostfs(&buf, p);
> 	return 0;
> }
> @@ -181,44 +364,60 @@ void close_dir(void *stream)
> 	closedir(stream);
> }
> 
> -int file_create(char *name, int mode)
> +int file_create(char *name, int mode, uid_t uid, gid_t gid, int mnt_use_xattr)
> {
> 	int fd;
> +	long ret;
> 
> 	fd = open64(name, O_CREAT | O_RDWR, mode);
> 	if (fd < 0)
> 		return -errno;
> +
> +	ret = is_set_gid(name, mnt_use_xattr);
> +	if (ret != -1)
> +		gid = ret;
> +	uml_chown(name, uid, gid, mnt_use_xattr);
> 	return fd;
> }
> 
> -int set_attr(const char *file, struct hostfs_iattr *attrs, int fd)
> +int set_attr(const char *file, struct hostfs_iattr *attrs, int fd,
> +							     int mnt_use_xattr)
> {
> 	struct hostfs_stat st;
> 	struct timeval times[2];
> -	int err, ma;
> +	int err, ma, ret;
> 
> 	if (attrs->ia_valid & HOSTFS_ATTR_MODE) {
> 		if (fd >= 0) {
> -			if (fchmod(fd, attrs->ia_mode) != 0)
> -				return -errno;
> -		} else if (chmod(file, attrs->ia_mode) != 0) {
> -			return -errno;
> +			ret = uml_fchmod(fd, attrs->ia_mode, mnt_use_xattr);
> +			if (ret != 0)
> +				return ret;
> +		} else {
> +			ret = uml_chmod(file, attrs->ia_mode, mnt_use_xattr);
> +			if (ret != 0)
> +				return ret;
> 		}
> 	}
> 	if (attrs->ia_valid & HOSTFS_ATTR_UID) {
> 		if (fd >= 0) {
> -			if (fchown(fd, attrs->ia_uid, -1))
> -				return -errno;
> -		} else if (chown(file, attrs->ia_uid, -1)) {
> -			return -errno;
> +			ret = uml_fchown(fd, attrs->ia_uid, -1, mnt_use_xattr);
> +			if (ret != 0)
> +				return ret;
> +		} else {
> +			ret = uml_chown(file, attrs->ia_uid, -1, mnt_use_xattr);
> +			if (ret != 0)
> +				return ret;
> 		}
> 	}
> 	if (attrs->ia_valid & HOSTFS_ATTR_GID) {
> 		if (fd >= 0) {
> -			if (fchown(fd, -1, attrs->ia_gid))
> -				return -errno;
> -		} else if (chown(file, -1, attrs->ia_gid)) {
> -			return -errno;
> +			ret = uml_fchown(fd, -1, attrs->ia_gid, mnt_use_xattr);
> +			if (ret != 0)
> +				return ret;
> +		} else {
> +			ret = uml_chown(file, -1, attrs->ia_gid, mnt_use_xattr);
> +			if (ret != 0)
> +				return ret;
> 		}
> 	}
> 	if (attrs->ia_valid & HOSTFS_ATTR_SIZE) {
> @@ -237,7 +436,7 @@ int set_attr(const char *file, struct hostfs_iattr *attrs,
> int fd)
> 	 */
> 	ma = (HOSTFS_ATTR_ATIME_SET | HOSTFS_ATTR_MTIME_SET);
> 	if (attrs->ia_valid & ma) {
> -		err = stat_file(file, &st, fd);
> +		err = stat_file(file, &st, fd, mnt_use_xattr);
> 		if (err != 0)
> 			return err;
> 
> @@ -265,7 +464,7 @@ int set_attr(const char *file, struct hostfs_iattr *attrs,
> int fd)
> 
> 	/* Note: ctime is not handled */
> 	if (attrs->ia_valid & (HOSTFS_ATTR_ATIME | HOSTFS_ATTR_MTIME)) {
> -		err = stat_file(file, &st, fd);
> +		err = stat_file(file, &st, fd, mnt_use_xattr);
> 		attrs->ia_atime = st.atime;
> 		attrs->ia_mtime = st.mtime;
> 		if (err != 0)
> @@ -294,13 +493,18 @@ int unlink_file(const char *file)
> 	return 0;
> }
> 
> -int do_mkdir(const char *file, int mode)
> +int do_mkdir(const char *file, int mode, uid_t uid, gid_t gid, int
> mnt_use_xattr)
> {
> 	int err;
> +	long ret;
> 
> 	err = mkdir(file, mode);
> 	if (err)
> 		return -errno;
> +	ret = is_set_gid(file, mnt_use_xattr);
> +	if (ret != -1)
> +		gid = ret;
> +	uml_chown(file, uid, gid, mnt_use_xattr);
> 	return 0;
> }
> 
> --
> 2.39.2

_______________________________________________
linux-um mailing list
linux-um@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-um