util-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs
@ 2021-11-24 18:26 Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 1/6] include/c: Add abs_diff macro Sean Anderson
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Sean Anderson @ 2021-11-24 18:26 UTC (permalink / raw)
  To: util-linux, Karel Zak
  Cc: Mikhail Gusarov, Matthew Harm Bekkema, James Peach, Sean Anderson

This series adds support for mapping ranges of user/group IDs using the
newuidmap and newgidmap programs from shadow. The intent is to allow
for root-less bootstrapping of Linux root filesystems with correct
ownership. My primary inspiration is mmdebstrap [1], which uses
unshare(2) to create Debian root filesystems without needing root
access.

[1] https://gitlab.mister-muffin.de/josch/mmdebstrap

Changes in v2:
- Add "auto" option for --map-users and --map-groups
- Add UID_BUFSIZ macro to hold the maximum size of a uid represented as
  a string
- Add some documentation for waitchild
- Add some helpers for forking and synchronizing
- Copy names from string_to_idarray into a buffer to add a
  nul-terminator, instead of modifying them directly
- Document new "auto" value for --map-user and --map-group
- Fix most of read_subid_range using spaces instead of tabs
- Fix typo of --group instead of --user
- Update doc comments for uint_to_id() and get_map_range()
- Use more meaningful numbers in map_ids
- Use pathname macros for /etc/sub{u,g}id
- Use sync helpers for idmap

Sean Anderson (6):
  include/c: Add abs_diff macro
  unshare: Add waitchild helper
  unshare: Add some helpers for forking and synchronizing
  unshare: Add options to map blocks of user/group IDs
  unshare: Add option to automatically create user and group maps
  unshare: Document --map-{groups,users,auto}

 include/c.h              |   8 +
 include/pathnames.h      |   3 +
 sys-utils/unshare.1.adoc |  32 +++
 sys-utils/unshare.c      | 477 ++++++++++++++++++++++++++++++++++-----
 4 files changed, 465 insertions(+), 55 deletions(-)

-- 
2.33.0


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/6] include/c: Add abs_diff macro
  2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
@ 2021-11-24 18:26 ` Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 2/6] unshare: Add waitchild helper Sean Anderson
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sean Anderson @ 2021-11-24 18:26 UTC (permalink / raw)
  To: util-linux, Karel Zak
  Cc: Mikhail Gusarov, Matthew Harm Bekkema, James Peach, Sean Anderson

This macro calculates abs(a - b). It is especially useful for unsigned
numbers.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

(no changes since v1)

 include/c.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/c.h b/include/c.h
index 354b59e29..ba799d8fd 100644
--- a/include/c.h
+++ b/include/c.h
@@ -159,6 +159,14 @@
 	_max1 > _max2 ? _max1 : _max2; })
 #endif
 
+#ifndef abs_diff
+# define abs_diff(x, y) __extension__ ({        \
+	__typeof__(x) _a = (x);			\
+	__typeof__(y) _b = (y);			\
+	(void) (&_a == &_b);			\
+	_a > _b ? _a - _b : _b - _a; })
+#endif
+
 #ifndef cmp_numbers
 # define cmp_numbers(x, y) __extension__ ({	\
 	__typeof__(x) _a = (x);			\
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/6] unshare: Add waitchild helper
  2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 1/6] include/c: Add abs_diff macro Sean Anderson
@ 2021-11-24 18:26 ` Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 3/6] unshare: Add some helpers for forking and synchronizing Sean Anderson
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sean Anderson @ 2021-11-24 18:26 UTC (permalink / raw)
  To: util-linux, Karel Zak
  Cc: Mikhail Gusarov, Matthew Harm Bekkema, James Peach, Sean Anderson

This refactors out the waitpid() logic into a function which will be
reused for the upcoming patches.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Add some documentation for waitchild

 sys-utils/unshare.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index 3f8287799..62fa66067 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -226,6 +226,30 @@ static void settime(time_t offset, clockid_t clk_id)
 	close(fd);
 }
 
+/**
+ * waitchild() - Wait for a process to exit successfully
+ * @pid: PID of the process to wait for
+ *
+ * Wait for a process to exit successfully. If it exits with a non-zero return
+ * code, then exit() with the same status.
+ */
+static void waitchild(int pid)
+{
+	int rc, status;
+
+	do {
+		rc = waitpid(pid, &status, 0);
+		if (rc < 0) {
+			if (errno == EINTR)
+				continue;
+			err(EXIT_FAILURE, _("waitpid failed"));
+		}
+		if (WIFEXITED(status) &&
+		    WEXITSTATUS(status) != EXIT_SUCCESS)
+			exit(WEXITSTATUS(status));
+	} while (rc < 0);
+}
+
 static void bind_ns_files_from_child(pid_t *child, int fds[2])
 {
 	char ch;
@@ -580,7 +604,6 @@ int main(int argc, char *argv[])
 	if (npersists && (pid || !forkit)) {
 		/* run in parent */
 		if (pid_bind && (unshare_flags & CLONE_NEWNS)) {
-			int rc;
 			char ch = PIPE_SYNC_BYTE;
 
 			/* signal child we are ready */
@@ -589,17 +612,7 @@ int main(int argc, char *argv[])
 			fds[1] = -1;
 
 			/* wait for bind_ns_files_from_child() */
-			do {
-				rc = waitpid(pid_bind, &status, 0);
-				if (rc < 0) {
-					if (errno == EINTR)
-						continue;
-					err(EXIT_FAILURE, _("waitpid failed"));
-				}
-				if (WIFEXITED(status) &&
-				    WEXITSTATUS(status) != EXIT_SUCCESS)
-					return WEXITSTATUS(status);
-			} while (rc < 0);
+			waitchild(pid_bind);
 		} else
 			/* simple way, just bind */
 			bind_ns_files(getpid());
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/6] unshare: Add some helpers for forking and synchronizing
  2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 1/6] include/c: Add abs_diff macro Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 2/6] unshare: Add waitchild helper Sean Anderson
@ 2021-11-24 18:26 ` Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 4/6] unshare: Add options to map blocks of user/group IDs Sean Anderson
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sean Anderson @ 2021-11-24 18:26 UTC (permalink / raw)
  To: util-linux, Karel Zak
  Cc: Mikhail Gusarov, Matthew Harm Bekkema, James Peach, Sean Anderson

There is (or rather, will be) a common pattern in unshare like

	/* parent */	/* child */
	fork()
	do_some_work()
	sync()		wait();
			do_more_work();
	wait()		exit();

where the parent has to do some tasks (unshare(), fork() again, etc)
before the child can do its work. At the moment this is implemented
explicitly with a pipe().

Add some helper functions to abstract this process away. In addition,
switch to eventfd() instead of pipe(). As the man page for eventfd(2)
notes,

> Applications can use an eventfd file descriptor instead of a pipe (see
> pipe(2)) in all cases where a pipe is used simply to signal events. The
> kernel overhead of an eventfd file descriptor is much lower than that of
> a pipe, and only one file descriptor is required (versus the two required
> for a pipe).

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- New

 sys-utils/unshare.c | 109 +++++++++++++++++++++++++++-----------------
 1 file changed, 68 insertions(+), 41 deletions(-)

diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index 62fa66067..f8229dfad 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -250,39 +250,74 @@ static void waitchild(int pid)
 	} while (rc < 0);
 }
 
-static void bind_ns_files_from_child(pid_t *child, int fds[2])
+/**
+ * sync_with_child() - Tell our child we're ready and wait for it to exit
+ * @pid: The pid of our child
+ * @fd: A file descriptor created with eventfd()
+ *
+ * This tells a child created with fork_and_wait() that we are ready for it to
+ * continue. Once we have done that, wait for our child to exit.
+ */
+static void sync_with_child(pid_t pid, int fd)
 {
-	char ch;
-	pid_t ppid = getpid();
-	ino_t ino = get_mnt_ino(ppid);
+	uint64_t ch = PIPE_SYNC_BYTE;
 
-	if (pipe(fds) < 0)
-		err(EXIT_FAILURE, _("pipe failed"));
+	write_all(fd, &ch, sizeof(ch));
+	close(fd);
 
-	*child = fork();
+	waitchild(pid);
+}
 
-	switch (*child) {
-	case -1:
+/**
+ * fork_and_wait() - Fork and wait to be sync'd with
+ * @fd - A file descriptor created with eventfd() which should be passed to
+ *       sync_with_child()
+ *
+ * This creates an eventfd and forks. The parent process returns immediately,
+ * but the child waits for a %PIPE_SYNC_BYTE on the eventfd before returning.
+ * This allows the parent to perform some tasks before the child starts its
+ * work. The parent should call sync_with_child() once it is ready for the
+ * child to continue.
+ *
+ * Return: The pid from fork()
+ */
+static pid_t fork_and_wait(int *fd)
+{
+	pid_t pid;
+	uint64_t ch;
+
+	*fd = eventfd(0, 0);
+	if (*fd < 0)
+		err(EXIT_FAILURE, _("eventfd failed"));
+
+	pid = fork();
+	if (pid < 0)
 		err(EXIT_FAILURE, _("fork failed"));
 
-	case 0:	/* child */
-		close(fds[1]);
-		fds[1] = -1;
-
-		/* wait for parent */
-		if (read_all(fds[0], &ch, 1) != 1 && ch != PIPE_SYNC_BYTE)
-			err(EXIT_FAILURE, _("failed to read pipe"));
-		if (get_mnt_ino(ppid) == ino)
-			exit(EXIT_FAILURE);
-		bind_ns_files(ppid);
-		exit(EXIT_SUCCESS);
-		break;
-
-	default: /* parent */
-		close(fds[0]);
-		fds[0] = -1;
-		break;
+	if (!pid) {
+		/* wait for the our parent to tell us to continue */
+		if (read_all(*fd, (char *)&ch, sizeof(ch)) != sizeof(ch) ||
+		    ch != PIPE_SYNC_BYTE)
+			err(EXIT_FAILURE, _("failed to read eventfd"));
+		close(*fd);
 	}
+
+	return pid;
+}
+
+static pid_t bind_ns_files_from_child(int *fd)
+{
+	pid_t child, ppid = getpid();
+	ino_t ino = get_mnt_ino(ppid);
+
+	child = fork_and_wait(fd);
+	if (child)
+		return child;
+
+	if (get_mnt_ino(ppid) == ino)
+		exit(EXIT_FAILURE);
+	bind_ns_files(ppid);
+	exit(EXIT_SUCCESS);
 }
 
 static uid_t get_user(const char *s, const char *err)
@@ -426,7 +461,7 @@ int main(int argc, char *argv[])
 	const char *newdir = NULL;
 	pid_t pid_bind = 0;
 	pid_t pid = 0;
-	int fds[2];
+	int fd_bind = -1;
 	int status;
 	unsigned long propagation = UNSHARE_PROPAGATION_DEFAULT;
 	int force_uid = 0, force_gid = 0;
@@ -570,7 +605,7 @@ int main(int argc, char *argv[])
 	signal(SIGCHLD, SIG_DFL);
 
 	if (npersists && (unshare_flags & CLONE_NEWNS))
-		bind_ns_files_from_child(&pid_bind, fds);
+		pid_bind = bind_ns_files_from_child(&fd_bind);
 
 	if (-1 == unshare(unshare_flags))
 		err(EXIT_FAILURE, _("unshare failed"));
@@ -593,8 +628,8 @@ int main(int argc, char *argv[])
 		case -1:
 			err(EXIT_FAILURE, _("fork failed"));
 		case 0:	/* child */
-			if (pid_bind && (unshare_flags & CLONE_NEWNS))
-				close(fds[1]);
+			if (npersists && (unshare_flags & CLONE_NEWNS))
+				close(fd_bind);
 			break;
 		default: /* parent */
 			break;
@@ -603,17 +638,9 @@ int main(int argc, char *argv[])
 
 	if (npersists && (pid || !forkit)) {
 		/* run in parent */
-		if (pid_bind && (unshare_flags & CLONE_NEWNS)) {
-			char ch = PIPE_SYNC_BYTE;
-
-			/* signal child we are ready */
-			write_all(fds[1], &ch, 1);
-			close(fds[1]);
-			fds[1] = -1;
-
-			/* wait for bind_ns_files_from_child() */
-			waitchild(pid_bind);
-		} else
+		if (pid_bind && (unshare_flags & CLONE_NEWNS))
+			sync_with_child(pid_bind, fd_bind);
+		else
 			/* simple way, just bind */
 			bind_ns_files(getpid());
 	}
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/6] unshare: Add options to map blocks of user/group IDs
  2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
                   ` (2 preceding siblings ...)
  2021-11-24 18:26 ` [PATCH v2 3/6] unshare: Add some helpers for forking and synchronizing Sean Anderson
@ 2021-11-24 18:26 ` Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 5/6] unshare: Add option to automatically create user and group maps Sean Anderson
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sean Anderson @ 2021-11-24 18:26 UTC (permalink / raw)
  To: util-linux, Karel Zak
  Cc: Mikhail Gusarov, Matthew Harm Bekkema, James Peach, Sean Anderson

This adds the ability to map multiple user/group IDs when creating a new
user namespace. Regular processes cannot map any user other than the
effective user, so we need to use the setuid helpers newuidmap and
newgidmap, provided by shadow. Typically, users will be assigned blocks
of user/group IDs in /etc/sub{u,g}id, although it is also possible to
use NSS. There is a second advantage in using these helpers: because we
never write to /proc/self/gid_map, we don't have to disable setgroups.

Because the process of mapping IDs is almost identical, whether we are
mapping user IDs or group IDs, we put both in a common "map_range"
structure. These are read in by (ab)using string_to_idarray. In addition
to any map created with --map-users, we still need to handle a map of
size one created with --map-user. This makes constructing the helpers'
command line the trickiest part of the whole process. newuidmap/
newgidmap check to see if any ranges overlap before creating a mapping.
To avoid failing, we carve out a hole in the mapping for the singular
map. In the worst case, we may have three separate maps.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Add UID_BUFSIZ macro to hold the maximum size of a uid represented as
  a string
- Copy names from string_to_idarray into a buffer to add a
  nul-terminator, instead of modifying them directly
- Update doc comments for uint_to_id() and get_map_range()
- Use more meaningful numbers in map_ids
- Use sync helpers for idmap

 sys-utils/unshare.c | 255 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 251 insertions(+), 4 deletions(-)

diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index f8229dfad..6d0a56334 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -24,6 +24,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
+#include <sys/eventfd.h>
 #include <sys/wait.h>
 #include <sys/mount.h>
 #include <sys/types.h>
@@ -356,6 +357,226 @@ static gid_t get_group(const char *s, const char *err)
 	return ret;
 }
 
+/**
+ * struct map_range - A range of IDs to map
+ * @outer: First ID inside the namespace
+ * @inner: First ID outside the namespace
+ * @count: Length of the inside and outside ranges
+ *
+ * A range of uids/gids to map using new[gu]idmap.
+ */
+struct map_range {
+	unsigned int outer;
+	unsigned int inner;
+	unsigned int count;
+};
+
+#define UID_BUFSIZ  sizeof(stringify_value(ULONG_MAX))
+
+/**
+ * uint_to_id() - Convert a string into a user/group ID
+ * @name: The string representation of the ID
+ * @sz: The length of @name, without an (optional) nul-terminator
+ *
+ * This converts a (possibly not nul-terminated_ string into user or group ID.
+ * No name lookup is performed.
+ *
+ * Return: @name as a numeric ID
+ */
+static int uint_to_id(const char *name, size_t sz)
+{
+	char buf[UID_BUFSIZ];
+
+	mem2strcpy(buf, name, sz, sizeof(buf));
+	return strtoul_or_err(name, _("could not parse ID"));
+}
+
+/**
+ * get_map_range() - Parse a mapping range from a string
+ * @s: A string of the format upper,lower,count
+ *
+ * Parse a string of the form upper,lower,count into a new mapping range.
+ *
+ * Return: A new &struct map_range
+ */
+static struct map_range *get_map_range(const char *s)
+{
+	int n, map[3];
+	struct map_range *ret;
+
+	n = string_to_idarray(s, map, ARRAY_SIZE(map), uint_to_id);
+	if (n < 0)
+		errx(EXIT_FAILURE, _("too many elements for mapping '%s'"), s);
+	if (n != ARRAY_SIZE(map))
+		errx(EXIT_FAILURE, _("mapping '%s' contains only %d elements"),
+		     s, n);
+
+	ret = xmalloc(sizeof(*ret));
+	ret->outer = map[0];
+	ret->inner = map[1];
+	ret->count = map[2];
+	return ret;
+}
+
+/**
+ * map_ids() - Create a new uid/gid map
+ * @idmapper: Either newuidmap or newgidmap
+ * @ppid: Pid to set the map for
+ * @outer: ID outside the namespace for a single map.
+ * @inner: ID inside the namespace for a single map. May be -1 to only use @map.
+ * @map: A range of IDs to map
+ *
+ * This creates a new uid/gid map for @ppid using @idmapper. The ID @outer in
+ * the parent (our) namespace is mapped to the ID @inner in the child (@ppid's)
+ * namespace. In addition, the range of IDs beginning at @map->outer is mapped
+ * to the range of IDs beginning at @map->inner. The tricky bit is that we
+ * cannot let these mappings overlap. We accomplish this by removing a "hole"
+ * from @map, if @outer or @inner overlap it. This may result in one less than
+ * @map->count IDs being mapped from @map. The unmapped IDs are always the
+ * topmost IDs of the mapping (either in the parent or the child namespace).
+ *
+ * Most of the time, this function will be called with @map->outer as some
+ * large ID, @map->inner as 0, and @map->count as a large number (at least
+ * 1000, but less than @map->outer). Typically, there will be no conflict with
+ * @outer. However, @inner may split the mapping for e.g. --map-current-user.
+ *
+ * This function always exec()s or errors out and does not return.
+ */
+static void __attribute__((__noreturn__))
+map_ids(const char *idmapper, int ppid, unsigned int outer, unsigned int inner,
+	struct map_range *map)
+{
+	/* idmapper + pid + 4 * map + NULL */
+	char *argv[15];
+	/* argv - idmapper - "1" - NULL */
+	char args[12][UID_BUFSIZ];
+	int i = 0, j = 0;
+	struct map_range lo, mid, hi;
+	unsigned int inner_offset, outer_offset;
+
+	/* Some helper macros to reduce bookkeeping */
+#define push_str(s) do { \
+	argv[i++] = s; \
+} while (0)
+#define push_ul(x) do { \
+	snprintf(args[j], sizeof(args[j]), "%u", x); \
+	push_str(args[j++]); \
+} while (0)
+
+	push_str(xstrdup(idmapper));
+	push_ul(ppid);
+	if ((int)inner == -1) {
+		/*
+		 * If we don't have a "single" mapping, then we can just use
+		 * map directly
+		 */
+		push_ul(map->inner);
+		push_ul(map->outer);
+		push_ul(map->count);
+		push_str(NULL);
+
+		execvp(idmapper, argv);
+		errexec(idmapper);
+	}
+
+	/* If the mappings overlap, remove an ID from map */
+	if ((outer >= map->outer && outer <= map->outer + map->count) ||
+	    (inner >= map->inner && inner <= map->inner + map->count))
+		map->count--;
+
+	/* Determine where the splits between lo, mid, and hi will be */
+	outer_offset = min(outer > map->outer ? outer - map->outer : 0,
+			   map->count);
+	inner_offset = min(inner > map->inner ? inner - map->inner : 0,
+			   map->count);
+
+	/*
+	 * In the worst case, we need three mappings:
+	 * From the bottom of map to either inner or outer
+	 */
+	lo.outer = map->outer;
+	lo.inner = map->inner;
+	lo.count = min(inner_offset, outer_offset);
+
+	/* From the lower of inner or outer to the higher */
+	mid.outer = lo.outer + lo.count;
+	mid.outer += mid.outer == outer;
+	mid.inner = lo.inner + lo.count;
+	mid.inner += mid.inner == inner;
+	mid.count = abs_diff(outer_offset, inner_offset);
+
+	/* And from the higher of inner or outer to the end of the map */
+	hi.outer = mid.outer + mid.count;
+	hi.outer += hi.outer == outer;
+	hi.inner = mid.inner + mid.count;
+	hi.inner += hi.inner == inner;
+	hi.count = map->count - lo.count - mid.count;
+
+	push_ul(inner);
+	push_ul(outer);
+	push_str("1");
+	/* new[gu]idmap doesn't like zero-length mappings, so skip them */
+	if (lo.count) {
+		push_ul(lo.inner);
+		push_ul(lo.outer);
+		push_ul(lo.count);
+	}
+	if (mid.count) {
+		push_ul(mid.inner);
+		push_ul(mid.outer);
+		push_ul(mid.count);
+	}
+	if (hi.count) {
+		push_ul(hi.inner);
+		push_ul(hi.outer);
+		push_ul(hi.count);
+	}
+	push_str(NULL);
+	execvp(idmapper, argv);
+	errexec(idmapper);
+}
+
+/**
+ * map_ids_from_child() - Set up a new uid/gid map
+ * @fd: The eventfd to wait on
+ * @mapuser: The user to map the current user to (or -1)
+ * @usermap: The range of UIDs to map (or %NULL)
+ * @mapgroup: The group to map the current group to (or -1)
+ * @groupmap: The range of GIDs to map (or %NULL)
+ *
+ * fork_and_wait() for our parent to call sync_with_child() on @fd. Upon
+ * recieving the go-ahead, use newuidmap and newgidmap to set the uid/gid map
+ * for our parent's PID.
+ *
+ * Return: The pid of the child.
+ */
+static pid_t map_ids_from_child(int *fd, uid_t mapuser,
+				struct map_range *usermap, gid_t mapgroup,
+				struct map_range *groupmap)
+{
+	pid_t child, pid = 0;
+	pid_t ppid = getpid();
+
+	child = fork_and_wait(fd);
+	if (child)
+		return child;
+
+	/* Avoid forking more than we need to */
+	if (usermap && groupmap) {
+		pid = fork();
+		if (pid < 0)
+			err(EXIT_FAILURE, _("fork failed"));
+		if (pid)
+			waitchild(pid);
+	}
+
+	if (!pid && usermap)
+		map_ids("newuidmap", ppid, geteuid(), mapuser, usermap);
+	if (groupmap)
+		map_ids("newgidmap", ppid, getegid(), mapgroup, groupmap);
+	exit(EXIT_SUCCESS);
+}
+
 static void __attribute__((__noreturn__)) usage(void)
 {
 	FILE *out = stdout;
@@ -382,6 +603,10 @@ static void __attribute__((__noreturn__)) usage(void)
 	fputs(_(" --map-group=<gid>|<name>  map current group to gid (implies --user)\n"), out);
 	fputs(_(" -r, --map-root-user       map current user to root (implies --user)\n"), out);
 	fputs(_(" -c, --map-current-user    map current user to itself (implies --user)\n"), out);
+	fputs(_(" --map-users=<outeruid>,<inneruid>,<count>\n"
+		"                           map count users from outeruid to inneruid (implies --user)\n"), out);
+	fputs(_(" --map-groups=<outergid>,<innergid>,<count>\n"
+		"                           map count groups from outergid to innergid (implies --user)\n"), out);
 	fputs(USAGE_SEPARATOR, out);
 	fputs(_(" --kill-child[=<signame>]  when dying, kill the forked child (implies --fork)\n"
 		"                             defaults to SIGKILL\n"), out);
@@ -416,7 +641,9 @@ int main(int argc, char *argv[])
 		OPT_MONOTONIC,
 		OPT_BOOTTIME,
 		OPT_MAPUSER,
+		OPT_MAPUSERS,
 		OPT_MAPGROUP,
+		OPT_MAPGROUPS,
 	};
 	static const struct option longopts[] = {
 		{ "help",          no_argument,       NULL, 'h'             },
@@ -435,7 +662,9 @@ int main(int argc, char *argv[])
 		{ "kill-child",    optional_argument, NULL, OPT_KILLCHILD   },
 		{ "mount-proc",    optional_argument, NULL, OPT_MOUNTPROC   },
 		{ "map-user",      required_argument, NULL, OPT_MAPUSER     },
+		{ "map-users",     required_argument, NULL, OPT_MAPUSERS    },
 		{ "map-group",     required_argument, NULL, OPT_MAPGROUP    },
+		{ "map-groups",    required_argument, NULL, OPT_MAPGROUPS   },
 		{ "map-root-user", no_argument,       NULL, 'r'             },
 		{ "map-current-user", no_argument,    NULL, 'c'             },
 		{ "propagation",   required_argument, NULL, OPT_PROPAGATION },
@@ -455,13 +684,15 @@ int main(int argc, char *argv[])
 	int c, forkit = 0;
 	uid_t mapuser = -1;
 	gid_t mapgroup = -1;
+	struct map_range *usermap = NULL;
+	struct map_range *groupmap = NULL;
 	int kill_child_signo = 0; /* 0 means --kill-child was not used */
 	const char *procmnt = NULL;
 	const char *newroot = NULL;
 	const char *newdir = NULL;
-	pid_t pid_bind = 0;
+	pid_t pid_bind = 0, pid_idmap = 0;
 	pid_t pid = 0;
-	int fd_bind = -1;
+	int fd_idmap, fd_bind = -1;
 	int status;
 	unsigned long propagation = UNSHARE_PROPAGATION_DEFAULT;
 	int force_uid = 0, force_gid = 0;
@@ -545,6 +776,14 @@ int main(int argc, char *argv[])
 			mapuser = real_euid;
 			mapgroup = real_egid;
 			break;
+		case OPT_MAPUSERS:
+			unshare_flags |= CLONE_NEWUSER;
+			usermap = get_map_range(optarg);
+			break;
+		case OPT_MAPGROUPS:
+			unshare_flags |= CLONE_NEWUSER;
+			groupmap = get_map_range(optarg);
+			break;
 		case OPT_SETGROUPS:
 			setgrpcmd = setgroups_str2id(optarg);
 			break;
@@ -607,9 +846,17 @@ int main(int argc, char *argv[])
 	if (npersists && (unshare_flags & CLONE_NEWNS))
 		pid_bind = bind_ns_files_from_child(&fd_bind);
 
+	if (usermap || groupmap)
+		pid_idmap = map_ids_from_child(&fd_idmap, mapuser, usermap,
+					       mapgroup, groupmap);
+
 	if (-1 == unshare(unshare_flags))
 		err(EXIT_FAILURE, _("unshare failed"));
 
+	/* Tell child we've called unshare() */
+	if (usermap || groupmap)
+		sync_with_child(pid_idmap, fd_idmap);
+
 	if (force_boottime)
 		settime(boottime, CLOCK_BOOTTIME);
 
@@ -662,14 +909,14 @@ int main(int argc, char *argv[])
 	if (kill_child_signo != 0 && prctl(PR_SET_PDEATHSIG, kill_child_signo) < 0)
 		err(EXIT_FAILURE, "prctl failed");
 
-        if (mapuser != (uid_t) -1)
+        if (mapuser != (uid_t) -1 && !usermap)
 		map_id(_PATH_PROC_UIDMAP, mapuser, real_euid);
 
         /* Since Linux 3.19 unprivileged writing of /proc/self/gid_map
          * has been disabled unless /proc/self/setgroups is written
          * first to permanently disable the ability to call setgroups
          * in that user namespace. */
-	if (mapgroup != (gid_t) -1) {
+	if (mapgroup != (gid_t) -1 && !groupmap) {
 		if (setgrpcmd == SETGROUPS_ALLOW)
 			errx(EXIT_FAILURE, _("options --setgroups=allow and "
 					"--map-group are mutually exclusive"));
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 5/6] unshare: Add option to automatically create user and group maps
  2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
                   ` (3 preceding siblings ...)
  2021-11-24 18:26 ` [PATCH v2 4/6] unshare: Add options to map blocks of user/group IDs Sean Anderson
@ 2021-11-24 18:26 ` Sean Anderson
  2021-11-24 18:26 ` [PATCH v2 6/6] unshare: Document --map-{groups,users,auto} Sean Anderson
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sean Anderson @ 2021-11-24 18:26 UTC (permalink / raw)
  To: util-linux, Karel Zak
  Cc: Mikhail Gusarov, Matthew Harm Bekkema, James Peach, Sean Anderson

This option is designed to handle the "garden path" user/group ID
mapping:

- The user has one big map in /etc/sub[u,g]id
- The user wants to map as many user and group IDs as they can,
  especially the first 1000 users and groups.

The "auto" map is designed to handle this. We find the first map
matching the current user, and then map the whole thing to the ID range
starting at ID 0.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Add "auto" option for --map-users and --map-groups
- Fix most of read_subid_range using spaces instead of tabs
- Use pathname macros for /etc/sub{u,g}id

 include/pathnames.h |  3 ++
 sys-utils/unshare.c | 84 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/include/pathnames.h b/include/pathnames.h
index 9be2baa83..5db815880 100644
--- a/include/pathnames.h
+++ b/include/pathnames.h
@@ -97,6 +97,9 @@
 #define _PATH_PROC_LOCKS        "/proc/locks"
 #define _PATH_PROC_CDROMINFO	"/proc/sys/dev/cdrom/info"
 
+/* unshare paths */
+#define _PATH_SUBUID		"/etc/subuid"
+#define _PATH_SUBGID		"/etc/subgid"
 #define _PATH_PROC_UIDMAP	"/proc/self/uid_map"
 #define _PATH_PROC_GIDMAP	"/proc/self/gid_map"
 #define _PATH_PROC_SETGROUPS	"/proc/self/setgroups"
diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index 6d0a56334..b5a18ed95 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -418,6 +418,72 @@ static struct map_range *get_map_range(const char *s)
 	return ret;
 }
 
+/**
+ * read_subid_range() - Look up a user's sub[gu]id range
+ * @filename: The file to look up the range from. This should be either
+ *            ``/etc/subuid`` or ``/etc/subgid``.
+ * @uid: The uid of the user whose range we should look up.
+ *
+ * This finds the first subid range matching @uid in @filename.
+ */
+static struct map_range *read_subid_range(char *filename, uid_t uid)
+{
+	char *line = NULL, *pwbuf;
+	FILE *idmap;
+	size_t n;
+	struct passwd *pw;
+	struct map_range *map;
+
+	map = xmalloc(sizeof(*map));
+	map->inner = 0;
+
+	pw = xgetpwuid(uid, &pwbuf);
+	if (!pw)
+		errx(EXIT_FAILURE, _("you (user %d) don't exist."), uid);
+
+	idmap = fopen(filename, "r");
+	if (!idmap)
+		err(EXIT_FAILURE, _("could not open '%s'"), filename);
+
+	/*
+	* Each line in sub[ug]idmap looks like
+	* username:subuid:count
+	* OR
+	* uid:subuid:count
+	*/
+	while (getline(&line, &n, idmap) != -1) {
+		char *rest, *s;
+
+		rest = strchr(line, ':');
+		if (!rest)
+			continue;
+		*rest = '\0';
+
+		if (strcmp(line, pw->pw_name) &&
+		    strtoul(line, NULL, 10) != pw->pw_uid)
+			continue;
+
+		s = rest + 1;
+		rest = strchr(s, ':');
+		if (!rest)
+			continue;
+		*rest = '\0';
+		map->outer = strtoul_or_err(s, _("failed to parse subid map"));
+
+		s = rest + 1;
+		rest = strchr(s, '\n');
+		if (rest)
+			*rest = '\0';
+		map->count = strtoul_or_err(s, _("failed to parse subid map"));
+
+		fclose(idmap);
+		return map;
+	}
+
+	err(EXIT_FAILURE, _("no line matching user \"%s\" in %s"),
+	pw->pw_name, filename);
+}
+
 /**
  * map_ids() - Create a new uid/gid map
  * @idmapper: Either newuidmap or newgidmap
@@ -603,6 +669,7 @@ static void __attribute__((__noreturn__)) usage(void)
 	fputs(_(" --map-group=<gid>|<name>  map current group to gid (implies --user)\n"), out);
 	fputs(_(" -r, --map-root-user       map current user to root (implies --user)\n"), out);
 	fputs(_(" -c, --map-current-user    map current user to itself (implies --user)\n"), out);
+	fputs(_(" --map-auto                map users and groups automatically (implies --user)\n"), out);
 	fputs(_(" --map-users=<outeruid>,<inneruid>,<count>\n"
 		"                           map count users from outeruid to inneruid (implies --user)\n"), out);
 	fputs(_(" --map-groups=<outergid>,<innergid>,<count>\n"
@@ -644,6 +711,7 @@ int main(int argc, char *argv[])
 		OPT_MAPUSERS,
 		OPT_MAPGROUP,
 		OPT_MAPGROUPS,
+		OPT_MAPAUTO,
 	};
 	static const struct option longopts[] = {
 		{ "help",          no_argument,       NULL, 'h'             },
@@ -667,6 +735,7 @@ int main(int argc, char *argv[])
 		{ "map-groups",    required_argument, NULL, OPT_MAPGROUPS   },
 		{ "map-root-user", no_argument,       NULL, 'r'             },
 		{ "map-current-user", no_argument,    NULL, 'c'             },
+		{ "map-auto",      no_argument,       NULL, OPT_MAPAUTO     },
 		{ "propagation",   required_argument, NULL, OPT_PROPAGATION },
 		{ "setgroups",     required_argument, NULL, OPT_SETGROUPS   },
 		{ "keep-caps",     no_argument,       NULL, OPT_KEEPCAPS    },
@@ -778,11 +847,22 @@ int main(int argc, char *argv[])
 			break;
 		case OPT_MAPUSERS:
 			unshare_flags |= CLONE_NEWUSER;
-			usermap = get_map_range(optarg);
+			if (!strcmp(optarg, "auto"))
+				usermap = read_subid_range(_PATH_SUBUID, real_euid);
+			else
+				usermap = get_map_range(optarg);
 			break;
 		case OPT_MAPGROUPS:
 			unshare_flags |= CLONE_NEWUSER;
-			groupmap = get_map_range(optarg);
+			if (!strcmp(optarg, "auto"))
+				groupmap = read_subid_range(_PATH_SUBGID, real_egid);
+			else
+				groupmap = get_map_range(optarg);
+			break;
+		case OPT_MAPAUTO:
+			unshare_flags |= CLONE_NEWUSER;
+			usermap = read_subid_range(_PATH_SUBUID, real_euid);
+			groupmap = read_subid_range(_PATH_SUBGID, real_egid);
 			break;
 		case OPT_SETGROUPS:
 			setgrpcmd = setgroups_str2id(optarg);
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 6/6] unshare: Document --map-{groups,users,auto}
  2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
                   ` (4 preceding siblings ...)
  2021-11-24 18:26 ` [PATCH v2 5/6] unshare: Add option to automatically create user and group maps Sean Anderson
@ 2021-11-24 18:26 ` Sean Anderson
  2021-12-01 15:16 ` [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Karel Zak
  2022-01-14 10:29 ` Daniel Gerber
  7 siblings, 0 replies; 13+ messages in thread
From: Sean Anderson @ 2021-11-24 18:26 UTC (permalink / raw)
  To: util-linux, Karel Zak
  Cc: Mikhail Gusarov, Matthew Harm Bekkema, James Peach, Sean Anderson

This documents the new options added in the previous few commits.
I have added another example to better demonstrate the these
options. The actual use is fairly straightforward, but the descriptions
are on the pithier side.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---

Changes in v2:
- Document new "auto" value for --map-user and --map-group
- Fix typo of --group instead of --user

 sys-utils/unshare.1.adoc | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/sys-utils/unshare.1.adoc b/sys-utils/unshare.1.adoc
index 74183ebc1..cb1302461 100644
--- a/sys-utils/unshare.1.adoc
+++ b/sys-utils/unshare.1.adoc
@@ -93,9 +93,18 @@ Just before running the program, mount the proc filesystem at _mountpoint_ (defa
 **--map-user=**__uid|name__::
 Run the program only after the current effective user ID has been mapped to _uid_. If this option is specified multiple times, the last occurrence takes precedence. This option implies *--user*.
 
+**--map-users=**__outeruid,inneruid,count__|**auto**::
+Run the program only after the block of user IDs of size _count_ beginning at _outeruid_ has been mapped to the block of user IDs beginning at _inneruid_. This mapping is created with **newuidmap**(1). If the range of user IDs overlaps with the mapping specified by *--map-user*, then a "hole" will be removed from the mapping. This may result in the highest user ID of the mapping not being mapped. The special value *auto* will map the first block of user IDs owned by the effective user from _/etc/subuid_ to a block starting at user ID 0. If this option is specified multiple times, the last occurrence takes precedence. This option implies *--user*.
+
 **--map-group=**__gid|name__::
 Run the program only after the current effective group ID has been mapped to _gid_. If this option is specified multiple times, the last occurrence takes precedence. This option implies *--setgroups=deny* and *--user*.
 
+**--map-groups=**__outergid,innergid,count__|**auto**::
+Run the program only after the block of group IDs of size _count_ beginning at _outergid_ has been mapped to the block of group IDs beginning at _innergid_. This mapping is created with **newgidmap**(1). If the range of group IDs overlaps with the mapping specified by *--map-group*, then a "hole" will be removed from the mapping. This may result in the highest group ID of the mapping not being mapped. The special value *auto* will map the first block of user IDs owned by the effective user from _/etc/subgid_ to a block starting at group ID 0. If this option is specified multiple times, the last occurrence takes precedence. This option implies *--user*.
+
+**--map-auto**::
+Map the first block of user IDs owned by the effective user from _/etc/subuid_ to a block starting at user ID 0. In the same manner, also map the first block of group IDs owned by the effective group from _/etc/subgid_ to a block starting at group ID 0. This option is intended to handle the common case where the first block of subordinate user and group IDs can map the whole user and group ID space. This option is equivalent to specifying *--map-users=auto* and *--map-groups=auto*.
+
 *-r*, *--map-root-user*::
 Run the program only after the current effective user and group IDs have been mapped to the superuser UID and GID in the newly created user namespace. This makes it possible to conveniently gain capabilities needed to manage various aspects of the newly created namespaces (such as configuring interfaces in the network namespace or mounting filesystems in the mount namespace) even when run unprivileged. As a mere convenience feature, it does not support more sophisticated use cases, such as mapping multiple ranges of UIDs and GIDs. This option implies *--setgroups=deny* and *--user*. This option is equivalent to *--map-user=0 --map-group=0*.
 
@@ -160,6 +169,27 @@ root
          0       1000          1
 ....
 
+As an unprivileged user, create a user namespace where the first 65536 IDs are all mapped, and the user's credentials are mapped to the root IDs inside the namespace. The map is determined by the subordinate IDs assigned in *subuid*(5) and *subgid*(5). Demonstrate this mapping by creating a file with user ID 1 and group ID 1. For brevity, only the user ID mappings are shown:
+
+....
+$ id -u
+1000
+$ cat /etc/subuid
+1000:100000:65536
+$ unshare --user --map-auto --map-root-user
+# id -u
+0
+# cat /proc/self/uid_map
+         0       1000          1
+         1     100000      65535
+# touch file; chown 1:1 file
+# ls -ln --time-style=+ file
+-rw-r--r-- 1 1 1 0  file
+# exit
+$ ls -ln --time-style=+ file
+-rw-r--r-- 1 100000 100000 0  file
+....
+
 The first of the following commands creates a new persistent UTS namespace and modifies the hostname as seen in that namespace. The namespace is then entered with *nsenter*(1) in order to display the modified hostname; this step demonstrates that the UTS namespace continues to exist even though the namespace had no member processes after the *unshare* command terminated. The namespace is then destroyed by removing the bind mount.
 
 ....
@@ -235,6 +265,8 @@ mailto:kzak@redhat.com[Karel Zak]
 
 == SEE ALSO
 
+*newuidmap*(1)
+*newgidmap*(1)
 *clone*(2),
 *unshare*(2),
 *namespaces*(7),
-- 
2.33.0


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs
  2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
                   ` (5 preceding siblings ...)
  2021-11-24 18:26 ` [PATCH v2 6/6] unshare: Document --map-{groups,users,auto} Sean Anderson
@ 2021-12-01 15:16 ` Karel Zak
  2022-01-14 10:29 ` Daniel Gerber
  7 siblings, 0 replies; 13+ messages in thread
From: Karel Zak @ 2021-12-01 15:16 UTC (permalink / raw)
  To: Sean Anderson
  Cc: util-linux, Mikhail Gusarov, Matthew Harm Bekkema, James Peach

On Wed, Nov 24, 2021 at 01:26:12PM -0500, Sean Anderson wrote:
> Sean Anderson (6):
>   include/c: Add abs_diff macro
>   unshare: Add waitchild helper
>   unshare: Add some helpers for forking and synchronizing
>   unshare: Add options to map blocks of user/group IDs
>   unshare: Add option to automatically create user and group maps
>   unshare: Document --map-{groups,users,auto}
> 
>  include/c.h              |   8 +
>  include/pathnames.h      |   3 +
>  sys-utils/unshare.1.adoc |  32 +++
>  sys-utils/unshare.c      | 477 ++++++++++++++++++++++++++++++++++-----
>  4 files changed, 465 insertions(+), 55 deletions(-)

Applied, thanks!

 Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs
  2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
                   ` (6 preceding siblings ...)
  2021-12-01 15:16 ` [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Karel Zak
@ 2022-01-14 10:29 ` Daniel Gerber
  2022-01-14 14:42   ` Sean Anderson
  2022-01-18 11:50   ` Karel Zak
  7 siblings, 2 replies; 13+ messages in thread
From: Daniel Gerber @ 2022-01-14 10:29 UTC (permalink / raw)
  To: Sean Anderson; +Cc: dottedmag, id, jpeach, kzak, util-linux

Hi,

Thanks for this feature. I've been trying it out... (This is with lib-musl-x86_64.)

Automatic mapping works:

$ unshare --map-users=auto cat /proc/self/uid_map
         0     100000      65536

But parsing id ranges does not:

$ unshare --map-users=100000,0,65536 cat /proc/self/uid_map
unshare: could not parse ID: '100000,0,65536'

Fix:
---
diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index 443358952..52bd9702a 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -388,7 +388,7 @@ static int uint_to_id(const char *name, size_t sz)
 	char buf[UID_BUFSIZ];

 	mem2strcpy(buf, name, sz, sizeof(buf));
-	return strtoul_or_err(name, _("could not parse ID"));
+	return strtoul_or_err(buf, _("could not parse ID"));
 }

 /**
---

Then, the value passed to newuidmap is still incorrect:

$ unshare --map-users=100000,0,65536 cat /proc/self/uid_map
newuidmap: uid range [0-655360) -> [100000-755360) not allowed

$ unshare --map-users=100000,0,0065536 cat /proc/self/uid_map
         0     100000      65536

The count value gets zero-padded to the right at some place I've not pinned down.


Also, I would suggest adopting the same argument order as in /proc/<pid>/uid_map and newuidmap -- inner,outer,count.

This doc string has it reversed:
---
/**
 * struct map_range - A range of IDs to map
 * @outer: First ID inside the namespace
 * @inner: First ID outside the namespace
---

And this one has inconsistent terminology:
---
 * get_map_range() - Parse a mapping range from a string
 * @s: A string of the format upper,lower,count
 *
 * Parse a string of the form upper,lower,count into a new mapping range.
---


--
Daniel Gerber

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs
  2022-01-14 10:29 ` Daniel Gerber
@ 2022-01-14 14:42   ` Sean Anderson
  2022-01-14 17:15     ` Daniel Gerber
  2022-01-18 11:50   ` Karel Zak
  1 sibling, 1 reply; 13+ messages in thread
From: Sean Anderson @ 2022-01-14 14:42 UTC (permalink / raw)
  To: Daniel Gerber; +Cc: dottedmag, id, jpeach, kzak, util-linux

On 1/14/22 5:29 AM, Daniel Gerber wrote:
> Hi,
> 
> Thanks for this feature. I've been trying it out... (This is with lib-musl-x86_64.)
> 
> Automatic mapping works:
> 
> $ unshare --map-users=auto cat /proc/self/uid_map
>           0     100000      65536
> 
> But parsing id ranges does not:
> 
> $ unshare --map-users=100000,0,65536 cat /proc/self/uid_map
> unshare: could not parse ID: '100000,0,65536'
> 
> Fix:
> ---
> diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
> index 443358952..52bd9702a 100644
> --- a/sys-utils/unshare.c
> +++ b/sys-utils/unshare.c
> @@ -388,7 +388,7 @@ static int uint_to_id(const char *name, size_t sz)
>   	char buf[UID_BUFSIZ];
> 
>   	mem2strcpy(buf, name, sz, sizeof(buf));
> -	return strtoul_or_err(name, _("could not parse ID"));
> +	return strtoul_or_err(buf, _("could not parse ID"));
>   }
> 
>   /**
> ---
> Then, the value passed to newuidmap is still incorrect:
> 
> $ unshare --map-users=100000,0,65536 cat /proc/self/uid_map
> newuidmap: uid range [0-655360) -> [100000-755360) not allowed
> 
> $ unshare --map-users=100000,0,0065536 cat /proc/self/uid_map
>           0     100000      65536
> 
> The count value gets zero-padded to the right at some place I've not pinned down.

It's stack garbage. Try

diff --git i/sys-utils/unshare.c w/sys-utils/unshare.c
index 3cdd90329..5ac7af3de 100644
--- i/sys-utils/unshare.c
+++ w/sys-utils/unshare.c
@@ -385,10 +385,10 @@ struct map_range {
   */
  static int uint_to_id(const char *name, size_t sz)
  {
-       char buf[UID_BUFSIZ];
+       char buf[UID_BUFSIZ] = {0};
  
-       mem2strcpy(buf, name, sz, sizeof(buf));
-       return strtoul_or_err(name, _("could not parse ID"));
+       memcpy(buf, name, min(sz, sizeof(buf) - 1));
+       return strtoul_or_err(buf, _("could not parse ID"));
  }
  
  /**
--

(actually, I have no idea what mem2strcpy is for if it doesn't put the nul-terminator at the end of sz)

> Also, I would suggest adopting the same argument order as in /proc/<pid>/uid_map and newuidmap -- inner,outer,count.

I think this is a rather silly order. Since this is a mapping, the "natural" order is

outer -> inner

and only from the new namespace's PoV is it

inner -> outer

It certainly helped me remember things once I reversed the order...

> This doc string has it reversed:

As noted above, this is intended.

> ---
> /**
>   * struct map_range - A range of IDs to map
>   * @outer: First ID inside the namespace
>   * @inner: First ID outside the namespace
> ---
> 
> And this one has inconsistent terminology:
> ---
>   * get_map_range() - Parse a mapping range from a string
>   * @s: A string of the format upper,lower,count
>   *
>   * Parse a string of the form upper,lower,count into a new mapping range.
> ---

And here you can see that I've been reading too much of shadow's man pages :)

--Sean

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs
  2022-01-14 14:42   ` Sean Anderson
@ 2022-01-14 17:15     ` Daniel Gerber
  2022-01-15  0:53       ` Sean Anderson
  0 siblings, 1 reply; 13+ messages in thread
From: Daniel Gerber @ 2022-01-14 17:15 UTC (permalink / raw)
  To: Sean Anderson; +Cc: dottedmag, id, jpeach, kzak, util-linux


2022-01-14, Sean Anderson:
> It's stack garbage. Try
>
> diff --git i/sys-utils/unshare.c w/sys-utils/unshare.c
> index 3cdd90329..5ac7af3de 100644
> --- i/sys-utils/unshare.c
> +++ w/sys-utils/unshare.c
> @@ -385,10 +385,10 @@ struct map_range {
>   */
>  static int uint_to_id(const char *name, size_t sz)
>  {
> -       char buf[UID_BUFSIZ];
> +       char buf[UID_BUFSIZ] = {0};
>  -       mem2strcpy(buf, name, sz, sizeof(buf));
> -       return strtoul_or_err(name, _("could not parse ID"));
> +       memcpy(buf, name, min(sz, sizeof(buf) - 1));
> +       return strtoul_or_err(buf, _("could not parse ID"));
>  }
>    /**

That works, thanks.


> > Also, I would suggest adopting the same argument order as in /proc/<pid>/uid_map and newuidmap -- inner,outer,count.

> I think this is a rather silly order. Since this is a mapping, the "natural" order is

> outer -> inner

> and only from the new namespace's PoV is it

> inner -> outer

> It certainly helped me remember things once I reversed the order...

All right, this may make some sense to me now. To the user discovering these tools though (me yesterday) the worst is missing one "standard" notation...

> > This doc string has it reversed:

> As noted above, this is intended.

> >   * struct map_range - A range of IDs to map
> >   * @outer: First ID inside the namespace
> >   * @inner: First ID outside the namespace

I mean "@outer: First ID inside ..." surely is a typo, isn't it?


Best,

Daniel

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs
  2022-01-14 17:15     ` Daniel Gerber
@ 2022-01-15  0:53       ` Sean Anderson
  0 siblings, 0 replies; 13+ messages in thread
From: Sean Anderson @ 2022-01-15  0:53 UTC (permalink / raw)
  To: Daniel Gerber; +Cc: dottedmag, id, jpeach, kzak, util-linux

On 1/14/22 12:15 PM, Daniel Gerber wrote:
> 
> 2022-01-14, Sean Anderson:
>> It's stack garbage. Try
>>
>> diff --git i/sys-utils/unshare.c w/sys-utils/unshare.c
>> index 3cdd90329..5ac7af3de 100644
>> --- i/sys-utils/unshare.c
>> +++ w/sys-utils/unshare.c
>> @@ -385,10 +385,10 @@ struct map_range {
>>    */
>>   static int uint_to_id(const char *name, size_t sz)
>>   {
>> -       char buf[UID_BUFSIZ];
>> +       char buf[UID_BUFSIZ] = {0};
>>   -       mem2strcpy(buf, name, sz, sizeof(buf));
>> -       return strtoul_or_err(name, _("could not parse ID"));
>> +       memcpy(buf, name, min(sz, sizeof(buf) - 1));
>> +       return strtoul_or_err(buf, _("could not parse ID"));
>>   }
>>     /**
> 
> That works, thanks.

OK, I will submit it.

> 
>>> Also, I would suggest adopting the same argument order as in /proc/<pid>/uid_map and newuidmap -- inner,outer,count.
> 
>> I think this is a rather silly order. Since this is a mapping, the "natural" order is
> 
>> outer -> inner
> 
>> and only from the new namespace's PoV is it
> 
>> inner -> outer
> 
>> It certainly helped me remember things once I reversed the order...
> 
> All right, this may make some sense to me now. To the user discovering these tools though (me yesterday) the worst is missing one "standard" notation...

Yeah, I'm not terribly happy about it. That's why I added an "auto" feature, so you wouldn't have to remember the order.

>>> This doc string has it reversed:
> 
>> As noted above, this is intended.
> 
>>>    * struct map_range - A range of IDs to map
>>>    * @outer: First ID inside the namespace
>>>    * @inner: First ID outside the namespace
> 
> I mean "@outer: First ID inside ..." surely is a typo, isn't it?

Yes, you're right.

--Sean

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs
  2022-01-14 10:29 ` Daniel Gerber
  2022-01-14 14:42   ` Sean Anderson
@ 2022-01-18 11:50   ` Karel Zak
  1 sibling, 0 replies; 13+ messages in thread
From: Karel Zak @ 2022-01-18 11:50 UTC (permalink / raw)
  To: Daniel Gerber; +Cc: Sean Anderson, dottedmag, id, jpeach, util-linux

On Fri, Jan 14, 2022 at 11:29:21AM +0100, Daniel Gerber wrote:
>  	mem2strcpy(buf, name, sz, sizeof(buf));
> -	return strtoul_or_err(name, _("could not parse ID"));
> +	return strtoul_or_err(buf, _("could not parse ID"));

Should be fixed in git tree. Thanks for your report!

 Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-01-18 11:51 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-24 18:26 [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Sean Anderson
2021-11-24 18:26 ` [PATCH v2 1/6] include/c: Add abs_diff macro Sean Anderson
2021-11-24 18:26 ` [PATCH v2 2/6] unshare: Add waitchild helper Sean Anderson
2021-11-24 18:26 ` [PATCH v2 3/6] unshare: Add some helpers for forking and synchronizing Sean Anderson
2021-11-24 18:26 ` [PATCH v2 4/6] unshare: Add options to map blocks of user/group IDs Sean Anderson
2021-11-24 18:26 ` [PATCH v2 5/6] unshare: Add option to automatically create user and group maps Sean Anderson
2021-11-24 18:26 ` [PATCH v2 6/6] unshare: Document --map-{groups,users,auto} Sean Anderson
2021-12-01 15:16 ` [PATCH v2 0/6] unshare: Add support for mapping ranges of user/group IDs Karel Zak
2022-01-14 10:29 ` Daniel Gerber
2022-01-14 14:42   ` Sean Anderson
2022-01-14 17:15     ` Daniel Gerber
2022-01-15  0:53       ` Sean Anderson
2022-01-18 11:50   ` Karel Zak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).