[PATCH 0/8] Open socket and PID files as root, before switching

[PATCH 0/8] Open socket and PID files as root, before switching

[Stefano Brivio]

If libguestfs tools run as root, with the 'direct' backend (without
libvirt), we'll start as root as well.

As guest images might be owned by root, there are valid reasons to use
libguestfs tools as root, so be nice to them: open socket and PID
files *before* switching to nobody, so that we can still access their
paths.

Stefano Brivio (8):
  conf: Don't lecture user about starting us as root
  tap: Move all-ones initialisation of mac_guest to tap_sock_init()
  passt, tap: Don't use -1 as uninitialised value for fd_tap_listen
  tap: Split tap_sock_unix_init() into opening and listening parts
  util: Rename write_pidfile() to pidfile_write()
  passt, util: Move opening of PID file to its own function
  conf, passt, tap: Open socket and PID files before switching UID/GID
  conf, passt.h: Rename pid_file in struct ctx to pidfile

 conf.c  | 23 +++++++++++++++++++----
 passt.c | 17 ++++-------------
 passt.h |  8 ++++++--
 tap.c   | 57 +++++++++++++++++++++++++++++++++++----------------------
 tap.h   |  1 +
 util.c  | 28 +++++++++++++++++++++++++---
 util.h  |  3 ++-
 7 files changed, 92 insertions(+), 45 deletions(-)

-- 
2.43.0

[PATCH 1/8] conf: Don't lecture user about starting us as root

[Stefano Brivio]

libguestfs tools have a good reason to run as root: if the guest image
is owned by root, it would be counterproductive to encourage users to
invoke them as non-root, as it would require changing permissions or
ownership of the image file.

And if they run as root, we'll start as root, too. Warn users we'll
switch to 'nobody', but don't tell them what to do.

Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conf.c b/conf.c
index 21d46fe..2e0d909 100644
--- a/conf.c
+++ b/conf.c
@@ -1093,7 +1093,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
 		return;
 
 	/* ...otherwise use nobody:nobody */
-	warn("Don't run as root. Changing to nobody...");
+	warn("Started as root. Changing to nobody...");
 	{
 #ifndef GLIBC_NO_STATIC_NSS
 		const struct passwd *pw;
-- 
@@ -1093,7 +1093,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
 		return;
 
 	/* ...otherwise use nobody:nobody */
-	warn("Don't run as root. Changing to nobody...");
+	warn("Started as root. Changing to nobody...");
 	{
 #ifndef GLIBC_NO_STATIC_NSS
 		const struct passwd *pw;
-- 
2.43.0

Re: [PATCH 1/8] conf: Don't lecture user about starting us as root

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On Wed, May 22, 2024 at 10:59:04PM +0200, Stefano Brivio wrote:
> libguestfs tools have a good reason to run as root: if the guest image
> is owned by root, it would be counterproductive to encourage users to
> invoke them as non-root, as it would require changing permissions or
> ownership of the image file.
> 
> And if they run as root, we'll start as root, too. Warn users we'll
> switch to 'nobody', but don't tell them what to do.
> 
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/conf.c b/conf.c
> index 21d46fe..2e0d909 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1093,7 +1093,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  		return;
>  
>  	/* ...otherwise use nobody:nobody */
> -	warn("Don't run as root. Changing to nobody...");
> +	warn("Started as root. Changing to nobody...");
>  	{
>  #ifndef GLIBC_NO_STATIC_NSS
>  		const struct passwd *pw;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/8] conf: Don't lecture user about starting us as root

[Richard W.M. Jones]

On Wed, May 22, 2024 at 10:59:04PM +0200, Stefano Brivio wrote:
> libguestfs tools have a good reason to run as root: if the guest image
> is owned by root, it would be counterproductive to encourage users to
> invoke them as non-root, as it would require changing permissions or
> ownership of the image file.
> 
> And if they run as root, we'll start as root, too. Warn users we'll
> switch to 'nobody', but don't tell them what to do.
> 
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  conf.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/conf.c b/conf.c
> index 21d46fe..2e0d909 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1093,7 +1093,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  		return;
>  
>  	/* ...otherwise use nobody:nobody */
> -	warn("Don't run as root. Changing to nobody...");
> +	warn("Started as root. Changing to nobody...");
>  	{
>  #ifndef GLIBC_NO_STATIC_NSS
>  		const struct passwd *pw;

Makes sense:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit

[PATCH 2/8] tap: Move all-ones initialisation of mac_guest to tap_sock_init()

[Stefano Brivio]

It has nothing to do with tap_sock_unix_init(). It used to be there as
that function could be called multiple times per passt instance, but
it's not the case anymore.

This also takes care of the fact that, with --fd, we wouldn't set the
initial MAC address, so we would need to wait for the guest to send us
an ARP packet before we could exchange data.

Fixes: 6b4e68383c66 ("passt, tap: Add --fd option")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tap.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tap.c b/tap.c
index 91fd2e2..177fe26 100644
--- a/tap.c
+++ b/tap.c
@@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c)
 	if (fd < 0)
 		die("UNIX socket: %s", strerror(errno));
 
-	/* In passt mode, we don't know the guest's MAC until it sends
-	 * us packets.  Use the broadcast address so our first packets
-	 * will reach it.
-	 */
-	memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
-
 	for (i = 1; i < UNIX_SOCK_MAX; i++) {
 		char *path = addr.sun_path;
 		int ex, ret;
@@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c)
 	if (c->mode == MODE_PASST) {
 		if (c->fd_tap_listen == -1)
 			tap_sock_unix_init(c);
+
+		/* In passt mode, we don't know the guest's MAC address until it
+		 * sends us packets.  Use the broadcast address so that our
+		 * first packets will reach it.
+		 */
+		memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
 	} else {
 		tap_sock_tun_init(c);
 	}
-- 
@@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c)
 	if (fd < 0)
 		die("UNIX socket: %s", strerror(errno));
 
-	/* In passt mode, we don't know the guest's MAC until it sends
-	 * us packets.  Use the broadcast address so our first packets
-	 * will reach it.
-	 */
-	memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
-
 	for (i = 1; i < UNIX_SOCK_MAX; i++) {
 		char *path = addr.sun_path;
 		int ex, ret;
@@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c)
 	if (c->mode == MODE_PASST) {
 		if (c->fd_tap_listen == -1)
 			tap_sock_unix_init(c);
+
+		/* In passt mode, we don't know the guest's MAC address until it
+		 * sends us packets.  Use the broadcast address so that our
+		 * first packets will reach it.
+		 */
+		memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
 	} else {
 		tap_sock_tun_init(c);
 	}
-- 
2.43.0

Re: [PATCH 2/8] tap: Move all-ones initialisation of mac_guest to tap_sock_init()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

On Wed, May 22, 2024 at 10:59:05PM +0200, Stefano Brivio wrote:
> It has nothing to do with tap_sock_unix_init(). It used to be there as
> that function could be called multiple times per passt instance, but
> it's not the case anymore.
> 
> This also takes care of the fact that, with --fd, we wouldn't set the
> initial MAC address, so we would need to wait for the guest to send us
> an ARP packet before we could exchange data.
> 
> Fixes: 6b4e68383c66 ("passt, tap: Add --fd option")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tap.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 91fd2e2..177fe26 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c)
>  	if (fd < 0)
>  		die("UNIX socket: %s", strerror(errno));
>  
> -	/* In passt mode, we don't know the guest's MAC until it sends
> -	 * us packets.  Use the broadcast address so our first packets
> -	 * will reach it.
> -	 */
> -	memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
> -
>  	for (i = 1; i < UNIX_SOCK_MAX; i++) {
>  		char *path = addr.sun_path;
>  		int ex, ret;
> @@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c)
>  	if (c->mode == MODE_PASST) {
>  		if (c->fd_tap_listen == -1)
>  			tap_sock_unix_init(c);
> +
> +		/* In passt mode, we don't know the guest's MAC address until it
> +		 * sends us packets.  Use the broadcast address so that our
> +		 * first packets will reach it.
> +		 */
> +		memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
>  	} else {
>  		tap_sock_tun_init(c);
>  	}

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/8] tap: Move all-ones initialisation of mac_guest to tap_sock_init()

[Richard W.M. Jones]

On Wed, May 22, 2024 at 10:59:05PM +0200, Stefano Brivio wrote:
> It has nothing to do with tap_sock_unix_init(). It used to be there as
> that function could be called multiple times per passt instance, but
> it's not the case anymore.
> 
> This also takes care of the fact that, with --fd, we wouldn't set the
> initial MAC address, so we would need to wait for the guest to send us
> an ARP packet before we could exchange data.
> 
> Fixes: 6b4e68383c66 ("passt, tap: Add --fd option")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  tap.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 91fd2e2..177fe26 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c)
>  	if (fd < 0)
>  		die("UNIX socket: %s", strerror(errno));
>  
> -	/* In passt mode, we don't know the guest's MAC until it sends
> -	 * us packets.  Use the broadcast address so our first packets
> -	 * will reach it.
> -	 */
> -	memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
> -
>  	for (i = 1; i < UNIX_SOCK_MAX; i++) {
>  		char *path = addr.sun_path;
>  		int ex, ret;
> @@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c)
>  	if (c->mode == MODE_PASST) {
>  		if (c->fd_tap_listen == -1)
>  			tap_sock_unix_init(c);
> +
> +		/* In passt mode, we don't know the guest's MAC address until it
> +		 * sends us packets.  Use the broadcast address so that our
> +		 * first packets will reach it.
> +		 */
> +		memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
>  	} else {
>  		tap_sock_tun_init(c);
>  	}

Reading tap.c, the effect of this is that memset will also be called
when c->fd_tap_listen is set (the --fd option).  As c cannot be NULL
and c->mac_guest exists, this seems safe.

Acked-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [PATCH 2/8] tap: Move all-ones initialisation of mac_guest to tap_sock_init()

[Richard W.M. Jones]

On Thu, May 23, 2024 at 10:59:32AM +0100, Richard W.M. Jones wrote:
> On Wed, May 22, 2024 at 10:59:05PM +0200, Stefano Brivio wrote:
> > It has nothing to do with tap_sock_unix_init(). It used to be there as
> > that function could be called multiple times per passt instance, but
> > it's not the case anymore.
> > 
> > This also takes care of the fact that, with --fd, we wouldn't set the
> > initial MAC address, so we would need to wait for the guest to send us
> > an ARP packet before we could exchange data.
> > 
> > Fixes: 6b4e68383c66 ("passt, tap: Add --fd option")
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> > ---
> >  tap.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tap.c b/tap.c
> > index 91fd2e2..177fe26 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -1111,12 +1111,6 @@ static void tap_sock_unix_init(struct ctx *c)
> >  	if (fd < 0)
> >  		die("UNIX socket: %s", strerror(errno));
> >  
> > -	/* In passt mode, we don't know the guest's MAC until it sends
> > -	 * us packets.  Use the broadcast address so our first packets
> > -	 * will reach it.
> > -	 */
> > -	memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
> > -
> >  	for (i = 1; i < UNIX_SOCK_MAX; i++) {
> >  		char *path = addr.sun_path;
> >  		int ex, ret;
> > @@ -1312,6 +1306,12 @@ void tap_sock_init(struct ctx *c)
> >  	if (c->mode == MODE_PASST) {
> >  		if (c->fd_tap_listen == -1)
> >  			tap_sock_unix_init(c);
> > +
> > +		/* In passt mode, we don't know the guest's MAC address until it
> > +		 * sends us packets.  Use the broadcast address so that our
> > +		 * first packets will reach it.
> > +		 */
> > +		memset(&c->mac_guest, 0xff, sizeof(c->mac_guest));
> >  	} else {
> >  		tap_sock_tun_init(c);
> >  	}
> 
> Reading tap.c, the effect of this is that memset will also be called
> when c->fd_tap_listen is set (the --fd option).  As c cannot be NULL

Reading the next patch, I see that fd_tap_listen *isn't* the --fd
option ...  However that doesn't change the analysis.

> and c->mac_guest exists, this seems safe.
> 
> Acked-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit

[PATCH 3/8] passt, tap: Don't use -1 as uninitialised value for fd_tap_listen

[Stefano Brivio]

This is a remnant from the time we kept access to the original
filesystem and we could reinitialise the listening AF_UNIX socket.

Since commit 0515adceaa8f ("passt, pasta: Namespace-based sandboxing,
defer seccomp policy application"), however, we can't re-bind the
listening socket once we're up and running.

Drop the -1 initalisation and the corresponding check.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 passt.c | 2 +-
 tap.c   | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/passt.c b/passt.c
index 771b8a7..1df1dc4 100644
--- a/passt.c
+++ b/passt.c
@@ -211,7 +211,7 @@ int main(int argc, char **argv)
 
 	isolate_initial();
 
-	c.pasta_netns_fd = c.fd_tap = c.fd_tap_listen = -1;
+	c.pasta_netns_fd = c.fd_tap = -1;
 
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
diff --git a/tap.c b/tap.c
index 177fe26..cb6df5a 100644
--- a/tap.c
+++ b/tap.c
@@ -1304,8 +1304,7 @@ void tap_sock_init(struct ctx *c)
 	}
 
 	if (c->mode == MODE_PASST) {
-		if (c->fd_tap_listen == -1)
-			tap_sock_unix_init(c);
+		tap_sock_unix_init(c);
 
 		/* In passt mode, we don't know the guest's MAC address until it
 		 * sends us packets.  Use the broadcast address so that our
-- 
@@ -1304,8 +1304,7 @@ void tap_sock_init(struct ctx *c)
 	}
 
 	if (c->mode == MODE_PASST) {
-		if (c->fd_tap_listen == -1)
-			tap_sock_unix_init(c);
+		tap_sock_unix_init(c);
 
 		/* In passt mode, we don't know the guest's MAC address until it
 		 * sends us packets.  Use the broadcast address so that our
-- 
2.43.0

Re: [PATCH 3/8] passt, tap: Don't use -1 as uninitialised value for fd_tap_listen

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

On Wed, May 22, 2024 at 10:59:06PM +0200, Stefano Brivio wrote:
> This is a remnant from the time we kept access to the original
> filesystem and we could reinitialise the listening AF_UNIX socket.
> 
> Since commit 0515adceaa8f ("passt, pasta: Namespace-based sandboxing,
> defer seccomp policy application"), however, we can't re-bind the
> listening socket once we're up and running.
> 
> Drop the -1 initalisation and the corresponding check.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  passt.c | 2 +-
>  tap.c   | 3 +--
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/passt.c b/passt.c
> index 771b8a7..1df1dc4 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -211,7 +211,7 @@ int main(int argc, char **argv)
>  
>  	isolate_initial();
>  
> -	c.pasta_netns_fd = c.fd_tap = c.fd_tap_listen = -1;
> +	c.pasta_netns_fd = c.fd_tap = -1;
>  
>  	sigemptyset(&sa.sa_mask);
>  	sa.sa_flags = 0;
> diff --git a/tap.c b/tap.c
> index 177fe26..cb6df5a 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1304,8 +1304,7 @@ void tap_sock_init(struct ctx *c)
>  	}
>  
>  	if (c->mode == MODE_PASST) {
> -		if (c->fd_tap_listen == -1)
> -			tap_sock_unix_init(c);
> +		tap_sock_unix_init(c);
>  
>  		/* In passt mode, we don't know the guest's MAC address until it
>  		 * sends us packets.  Use the broadcast address so that our

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 4/8] tap: Split tap_sock_unix_init() into opening and listening parts

[Stefano Brivio]

We'll need to open and bind the socket a while before listening to it,
so split that into two different functions. No functional changes
intended.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 tap.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/tap.c b/tap.c
index cb6df5a..c9f580e 100644
--- a/tap.c
+++ b/tap.c
@@ -1095,14 +1095,14 @@ restart:
 }
 
 /**
- * tap_sock_unix_init() - Create and bind AF_UNIX socket, listen for connection
- * @c:		Execution context
+ * tap_sock_unix_open() - Create and bind AF_UNIX socket
+ * @sock_path:	Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix)
+ *
+ * Return: socket descriptor on success, won't return on failure
  */
-static void tap_sock_unix_init(struct ctx *c)
+static int tap_sock_unix_open(char *sock_path)
 {
 	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
-	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
-	struct epoll_event ev = { 0 };
 	struct sockaddr_un addr = {
 		.sun_family = AF_UNIX,
 	};
@@ -1115,8 +1115,8 @@ static void tap_sock_unix_init(struct ctx *c)
 		char *path = addr.sun_path;
 		int ex, ret;
 
-		if (*c->sock_path)
-			memcpy(path, c->sock_path, UNIX_PATH_MAX);
+		if (*sock_path)
+			memcpy(path, sock_path, UNIX_PATH_MAX);
 		else
 			snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
 
@@ -1127,7 +1127,7 @@ static void tap_sock_unix_init(struct ctx *c)
 		ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
 		if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
 			     errno != EACCES)) {
-			if (*c->sock_path)
+			if (*sock_path)
 				die("Socket path %s already in use", path);
 
 			close(ex);
@@ -1137,7 +1137,7 @@ static void tap_sock_unix_init(struct ctx *c)
 
 		unlink(path);
 		if (!bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) ||
-		    *c->sock_path)
+		    *sock_path)
 			break;
 	}
 
@@ -1145,17 +1145,31 @@ static void tap_sock_unix_init(struct ctx *c)
 		die("UNIX socket bind: %s", strerror(errno));
 
 	info("UNIX domain socket bound at %s\n", addr.sun_path);
+	if (!*sock_path)
+		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
+
+	return fd;
+}
+
+/**
+ * tap_sock_unix_init() - Start listening for connections on AF_UNIX socket
+ * @c:		Execution context
+ */
+static void tap_sock_unix_init(struct ctx *c)
+{
+	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
+	struct epoll_event ev = { 0 };
 
-	listen(fd, 0);
+	listen(c->fd_tap_listen, 0);
 
-	ref.fd = c->fd_tap_listen = fd;
+	ref.fd = c->fd_tap_listen;
 	ev.events = EPOLLIN | EPOLLET;
 	ev.data.u64 = ref.u64;
 	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
 
 	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
 	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
-	     addr.sun_path);
+	     c->sock_path);
 	info("or qrap, for earlier qemu versions:");
 	info("    ./qrap 5 kvm ... -net socket,fd=5 -net nic,model=virtio");
 }
@@ -1304,6 +1318,7 @@ void tap_sock_init(struct ctx *c)
 	}
 
 	if (c->mode == MODE_PASST) {
+		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
 		tap_sock_unix_init(c);
 
 		/* In passt mode, we don't know the guest's MAC address until it
-- 
@@ -1095,14 +1095,14 @@ restart:
 }
 
 /**
- * tap_sock_unix_init() - Create and bind AF_UNIX socket, listen for connection
- * @c:		Execution context
+ * tap_sock_unix_open() - Create and bind AF_UNIX socket
+ * @sock_path:	Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix)
+ *
+ * Return: socket descriptor on success, won't return on failure
  */
-static void tap_sock_unix_init(struct ctx *c)
+static int tap_sock_unix_open(char *sock_path)
 {
 	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
-	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
-	struct epoll_event ev = { 0 };
 	struct sockaddr_un addr = {
 		.sun_family = AF_UNIX,
 	};
@@ -1115,8 +1115,8 @@ static void tap_sock_unix_init(struct ctx *c)
 		char *path = addr.sun_path;
 		int ex, ret;
 
-		if (*c->sock_path)
-			memcpy(path, c->sock_path, UNIX_PATH_MAX);
+		if (*sock_path)
+			memcpy(path, sock_path, UNIX_PATH_MAX);
 		else
 			snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
 
@@ -1127,7 +1127,7 @@ static void tap_sock_unix_init(struct ctx *c)
 		ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
 		if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
 			     errno != EACCES)) {
-			if (*c->sock_path)
+			if (*sock_path)
 				die("Socket path %s already in use", path);
 
 			close(ex);
@@ -1137,7 +1137,7 @@ static void tap_sock_unix_init(struct ctx *c)
 
 		unlink(path);
 		if (!bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) ||
-		    *c->sock_path)
+		    *sock_path)
 			break;
 	}
 
@@ -1145,17 +1145,31 @@ static void tap_sock_unix_init(struct ctx *c)
 		die("UNIX socket bind: %s", strerror(errno));
 
 	info("UNIX domain socket bound at %s\n", addr.sun_path);
+	if (!*sock_path)
+		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
+
+	return fd;
+}
+
+/**
+ * tap_sock_unix_init() - Start listening for connections on AF_UNIX socket
+ * @c:		Execution context
+ */
+static void tap_sock_unix_init(struct ctx *c)
+{
+	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
+	struct epoll_event ev = { 0 };
 
-	listen(fd, 0);
+	listen(c->fd_tap_listen, 0);
 
-	ref.fd = c->fd_tap_listen = fd;
+	ref.fd = c->fd_tap_listen;
 	ev.events = EPOLLIN | EPOLLET;
 	ev.data.u64 = ref.u64;
 	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
 
 	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
 	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
-	     addr.sun_path);
+	     c->sock_path);
 	info("or qrap, for earlier qemu versions:");
 	info("    ./qrap 5 kvm ... -net socket,fd=5 -net nic,model=virtio");
 }
@@ -1304,6 +1318,7 @@ void tap_sock_init(struct ctx *c)
 	}
 
 	if (c->mode == MODE_PASST) {
+		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
 		tap_sock_unix_init(c);
 
 		/* In passt mode, we don't know the guest's MAC address until it
-- 
2.43.0

Re: [PATCH 4/8] tap: Split tap_sock_unix_init() into opening and listening parts

[Richard W.M. Jones]

On Wed, May 22, 2024 at 10:59:07PM +0200, Stefano Brivio wrote:
> We'll need to open and bind the socket a while before listening to it,
> so split that into two different functions. No functional changes
> intended.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  tap.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index cb6df5a..c9f580e 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1095,14 +1095,14 @@ restart:
>  }
>  
>  /**
> - * tap_sock_unix_init() - Create and bind AF_UNIX socket, listen for connection
> - * @c:		Execution context
> + * tap_sock_unix_open() - Create and bind AF_UNIX socket
> + * @sock_path:	Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix)
> + *
> + * Return: socket descriptor on success, won't return on failure
>   */
> -static void tap_sock_unix_init(struct ctx *c)
> +static int tap_sock_unix_open(char *sock_path)
>  {
>  	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
> -	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
> -	struct epoll_event ev = { 0 };
>  	struct sockaddr_un addr = {
>  		.sun_family = AF_UNIX,
>  	};
> @@ -1115,8 +1115,8 @@ static void tap_sock_unix_init(struct ctx *c)
>  		char *path = addr.sun_path;
>  		int ex, ret;
>  
> -		if (*c->sock_path)
> -			memcpy(path, c->sock_path, UNIX_PATH_MAX);
> +		if (*sock_path)
> +			memcpy(path, sock_path, UNIX_PATH_MAX);
>  		else
>  			snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
>  
> @@ -1127,7 +1127,7 @@ static void tap_sock_unix_init(struct ctx *c)
>  		ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
>  		if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
>  			     errno != EACCES)) {
> -			if (*c->sock_path)
> +			if (*sock_path)
>  				die("Socket path %s already in use", path);
>  
>  			close(ex);
> @@ -1137,7 +1137,7 @@ static void tap_sock_unix_init(struct ctx *c)
>  
>  		unlink(path);
>  		if (!bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) ||
> -		    *c->sock_path)
> +		    *sock_path)
>  			break;
>  	}
>  
> @@ -1145,17 +1145,31 @@ static void tap_sock_unix_init(struct ctx *c)
>  		die("UNIX socket bind: %s", strerror(errno));
>  
>  	info("UNIX domain socket bound at %s\n", addr.sun_path);
> +	if (!*sock_path)
> +		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
> +
> +	return fd;
> +}
> +
> +/**
> + * tap_sock_unix_init() - Start listening for connections on AF_UNIX socket
> + * @c:		Execution context
> + */
> +static void tap_sock_unix_init(struct ctx *c)
> +{
> +	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
> +	struct epoll_event ev = { 0 };
>  
> -	listen(fd, 0);
> +	listen(c->fd_tap_listen, 0);
>  
> -	ref.fd = c->fd_tap_listen = fd;
> +	ref.fd = c->fd_tap_listen;
>  	ev.events = EPOLLIN | EPOLLET;
>  	ev.data.u64 = ref.u64;
>  	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
>  
>  	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
>  	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
> -	     addr.sun_path);
> +	     c->sock_path);
>  	info("or qrap, for earlier qemu versions:");
>  	info("    ./qrap 5 kvm ... -net socket,fd=5 -net nic,model=virtio");
>  }
> @@ -1304,6 +1318,7 @@ void tap_sock_init(struct ctx *c)
>  	}
>  
>  	if (c->mode == MODE_PASST) {
> +		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
>  		tap_sock_unix_init(c);
>  
>  		/* In passt mode, we don't know the guest's MAC address until it
> -- 

Looks like a neutral factoring, so:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [PATCH 4/8] tap: Split tap_sock_unix_init() into opening and listening parts

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3905 bytes --]

On Wed, May 22, 2024 at 10:59:07PM +0200, Stefano Brivio wrote:
> We'll need to open and bind the socket a while before listening to it,
> so split that into two different functions. No functional changes
> intended.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tap.c | 39 +++++++++++++++++++++++++++------------
>  1 file changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index cb6df5a..c9f580e 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1095,14 +1095,14 @@ restart:
>  }
>  
>  /**
> - * tap_sock_unix_init() - Create and bind AF_UNIX socket, listen for connection
> - * @c:		Execution context
> + * tap_sock_unix_open() - Create and bind AF_UNIX socket
> + * @sock_path:	Socket path. If empty, set on return (UNIX_SOCK_PATH as prefix)
> + *
> + * Return: socket descriptor on success, won't return on failure
>   */
> -static void tap_sock_unix_init(struct ctx *c)
> +static int tap_sock_unix_open(char *sock_path)
>  {
>  	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
> -	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
> -	struct epoll_event ev = { 0 };
>  	struct sockaddr_un addr = {
>  		.sun_family = AF_UNIX,
>  	};
> @@ -1115,8 +1115,8 @@ static void tap_sock_unix_init(struct ctx *c)
>  		char *path = addr.sun_path;
>  		int ex, ret;
>  
> -		if (*c->sock_path)
> -			memcpy(path, c->sock_path, UNIX_PATH_MAX);
> +		if (*sock_path)
> +			memcpy(path, sock_path, UNIX_PATH_MAX);
>  		else
>  			snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
>  
> @@ -1127,7 +1127,7 @@ static void tap_sock_unix_init(struct ctx *c)
>  		ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
>  		if (!ret || (errno != ENOENT && errno != ECONNREFUSED &&
>  			     errno != EACCES)) {
> -			if (*c->sock_path)
> +			if (*sock_path)
>  				die("Socket path %s already in use", path);
>  
>  			close(ex);
> @@ -1137,7 +1137,7 @@ static void tap_sock_unix_init(struct ctx *c)
>  
>  		unlink(path);
>  		if (!bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) ||
> -		    *c->sock_path)
> +		    *sock_path)
>  			break;
>  	}
>  
> @@ -1145,17 +1145,31 @@ static void tap_sock_unix_init(struct ctx *c)
>  		die("UNIX socket bind: %s", strerror(errno));
>  
>  	info("UNIX domain socket bound at %s\n", addr.sun_path);
> +	if (!*sock_path)
> +		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
> +
> +	return fd;
> +}
> +
> +/**
> + * tap_sock_unix_init() - Start listening for connections on AF_UNIX socket
> + * @c:		Execution context
> + */
> +static void tap_sock_unix_init(struct ctx *c)
> +{
> +	union epoll_ref ref = { .type = EPOLL_TYPE_TAP_LISTEN };
> +	struct epoll_event ev = { 0 };
>  
> -	listen(fd, 0);
> +	listen(c->fd_tap_listen, 0);
>  
> -	ref.fd = c->fd_tap_listen = fd;
> +	ref.fd = c->fd_tap_listen;
>  	ev.events = EPOLLIN | EPOLLET;
>  	ev.data.u64 = ref.u64;
>  	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
>  
>  	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
>  	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
> -	     addr.sun_path);
> +	     c->sock_path);
>  	info("or qrap, for earlier qemu versions:");
>  	info("    ./qrap 5 kvm ... -net socket,fd=5 -net nic,model=virtio");
>  }
> @@ -1304,6 +1318,7 @@ void tap_sock_init(struct ctx *c)
>  	}
>  
>  	if (c->mode == MODE_PASST) {
> +		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
>  		tap_sock_unix_init(c);
>  
>  		/* In passt mode, we don't know the guest's MAC address until it

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 5/8] util: Rename write_pidfile() to pidfile_write()

[Stefano Brivio]

As I'm adding pidfile_open() in the next patch. The function comment
didn't match, by the way.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 passt.c | 2 +-
 util.c  | 6 +++---
 util.h  | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/passt.c b/passt.c
index 1df1dc4..fb9773d 100644
--- a/passt.c
+++ b/passt.c
@@ -317,7 +317,7 @@ int main(int argc, char **argv)
 	if (!c.foreground)
 		__daemon(pidfile_fd, devnull_fd);
 	else
-		write_pidfile(pidfile_fd, getpid());
+		pidfile_write(pidfile_fd, getpid());
 
 	if (pasta_child_pid)
 		kill(pasta_child_pid, SIGUSR1);
diff --git a/util.c b/util.c
index 849fa7f..18c04ba 100644
--- a/util.c
+++ b/util.c
@@ -380,11 +380,11 @@ int open_in_ns(const struct ctx *c, const char *path, int flags)
 }
 
 /**
- * pid_file() - Write PID to file, if requested to do so, and close it
+ * pidfile_write() - Write PID to file, if requested to do so, and close it
  * @fd:		Open PID file descriptor, closed on exit, -1 to skip writing it
  * @pid:	PID value to write
  */
-void write_pidfile(int fd, pid_t pid)
+void pidfile_write(int fd, pid_t pid)
 {
 	char pid_buf[12];
 	int n;
@@ -419,7 +419,7 @@ int __daemon(int pidfile_fd, int devnull_fd)
 	}
 
 	if (pid) {
-		write_pidfile(pidfile_fd, pid);
+		pidfile_write(pidfile_fd, pid);
 		exit(EXIT_SUCCESS);
 	}
 
diff --git a/util.h b/util.h
index 264423b..f811975 100644
--- a/util.h
+++ b/util.h
@@ -156,7 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
 void ns_enter(const struct ctx *c);
 bool ns_is_init(void);
 int open_in_ns(const struct ctx *c, const char *path, int flags);
-void write_pidfile(int fd, pid_t pid);
+void pidfile_write(int fd, pid_t pid);
 int __daemon(int pidfile_fd, int devnull_fd);
 int fls(unsigned long x);
 int write_file(const char *path, const char *buf);
-- 
@@ -156,7 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
 void ns_enter(const struct ctx *c);
 bool ns_is_init(void);
 int open_in_ns(const struct ctx *c, const char *path, int flags);
-void write_pidfile(int fd, pid_t pid);
+void pidfile_write(int fd, pid_t pid);
 int __daemon(int pidfile_fd, int devnull_fd);
 int fls(unsigned long x);
 int write_file(const char *path, const char *buf);
-- 
2.43.0

Re: [PATCH 5/8] util: Rename write_pidfile() to pidfile_write()

[Richard W.M. Jones]

On Wed, May 22, 2024 at 10:59:08PM +0200, Stefano Brivio wrote:
> As I'm adding pidfile_open() in the next patch. The function comment
> didn't match, by the way.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  passt.c | 2 +-
>  util.c  | 6 +++---
>  util.h  | 2 +-
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/passt.c b/passt.c
> index 1df1dc4..fb9773d 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -317,7 +317,7 @@ int main(int argc, char **argv)
>  	if (!c.foreground)
>  		__daemon(pidfile_fd, devnull_fd);
>  	else
> -		write_pidfile(pidfile_fd, getpid());
> +		pidfile_write(pidfile_fd, getpid());
>  
>  	if (pasta_child_pid)
>  		kill(pasta_child_pid, SIGUSR1);
> diff --git a/util.c b/util.c
> index 849fa7f..18c04ba 100644
> --- a/util.c
> +++ b/util.c
> @@ -380,11 +380,11 @@ int open_in_ns(const struct ctx *c, const char *path, int flags)
>  }
>  
>  /**
> - * pid_file() - Write PID to file, if requested to do so, and close it
> + * pidfile_write() - Write PID to file, if requested to do so, and close it
>   * @fd:		Open PID file descriptor, closed on exit, -1 to skip writing it
>   * @pid:	PID value to write
>   */
> -void write_pidfile(int fd, pid_t pid)
> +void pidfile_write(int fd, pid_t pid)
>  {
>  	char pid_buf[12];
>  	int n;
> @@ -419,7 +419,7 @@ int __daemon(int pidfile_fd, int devnull_fd)
>  	}
>  
>  	if (pid) {
> -		write_pidfile(pidfile_fd, pid);
> +		pidfile_write(pidfile_fd, pid);
>  		exit(EXIT_SUCCESS);
>  	}
>  
> diff --git a/util.h b/util.h
> index 264423b..f811975 100644
> --- a/util.h
> +++ b/util.h
> @@ -156,7 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
>  void ns_enter(const struct ctx *c);
>  bool ns_is_init(void);
>  int open_in_ns(const struct ctx *c, const char *path, int flags);
> -void write_pidfile(int fd, pid_t pid);
> +void pidfile_write(int fd, pid_t pid);
>  int __daemon(int pidfile_fd, int devnull_fd);
>  int fls(unsigned long x);
>  int write_file(const char *path, const char *buf);

Neutral refactoring, so:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

However the same function name appears (just as a comment) in
contrib/apparmor/usr.bin.passt so I don't know if you would want to
update that?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

[PATCH 6/8] passt, util: Move opening of PID file to its own function

[Stefano Brivio]

We won't call it from main() any longer: move it.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 passt.c | 11 ++---------
 util.c  | 22 ++++++++++++++++++++++
 util.h  |  1 +
 3 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/passt.c b/passt.c
index fb9773d..e2446fc 100644
--- a/passt.c
+++ b/passt.c
@@ -199,7 +199,7 @@ void exit_handler(int signal)
  */
 int main(int argc, char **argv)
 {
-	int nfds, i, devnull_fd = -1, pidfile_fd = -1;
+	int nfds, i, devnull_fd = -1, pidfile_fd;
 	struct epoll_event events[EPOLL_EVENTS];
 	char *log_name, argv0[PATH_MAX], *name;
 	struct ctx c = { 0 };
@@ -299,14 +299,7 @@ int main(int argc, char **argv)
 		}
 	}
 
-	if (*c.pid_file) {
-		if ((pidfile_fd = open(c.pid_file,
-				       O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
-				       S_IRUSR | S_IWUSR)) < 0) {
-			perror("PID file open");
-			exit(EXIT_FAILURE);
-		}
-	}
+	pidfile_fd = pidfile_open(c.pid_file);
 
 	if (isolate_prefork(&c))
 		die("Failed to sandbox process, exiting");
diff --git a/util.c b/util.c
index 18c04ba..cf5a62b 100644
--- a/util.c
+++ b/util.c
@@ -402,6 +402,28 @@ void pidfile_write(int fd, pid_t pid)
 	close(fd);
 }
 
+/**
+ * pidfile_open() - Open PID file if needed
+ * @path:	Path for PID file, empty string if no PID file is requested
+ *
+ * Return: descriptor for PID file, -1 if path is NULL, won't return on failure
+ */
+int pidfile_open(const char *path)
+{
+	int fd;
+
+	if (!*path)
+		return -1;
+
+	if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
+			     S_IRUSR | S_IWUSR)) < 0) {
+		perror("PID file open");
+		exit(EXIT_FAILURE);
+	}
+
+	return fd;
+}
+
 /**
  * __daemon() - daemon()-like function writing PID file before parent exits
  * @pidfile_fd:	Open PID file descriptor
diff --git a/util.h b/util.h
index f811975..8a430ca 100644
--- a/util.h
+++ b/util.h
@@ -156,6 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
 void ns_enter(const struct ctx *c);
 bool ns_is_init(void);
 int open_in_ns(const struct ctx *c, const char *path, int flags);
+int pidfile_open(const char *path);
 void pidfile_write(int fd, pid_t pid);
 int __daemon(int pidfile_fd, int devnull_fd);
 int fls(unsigned long x);
-- 
@@ -156,6 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
 void ns_enter(const struct ctx *c);
 bool ns_is_init(void);
 int open_in_ns(const struct ctx *c, const char *path, int flags);
+int pidfile_open(const char *path);
 void pidfile_write(int fd, pid_t pid);
 int __daemon(int pidfile_fd, int devnull_fd);
 int fls(unsigned long x);
-- 
2.43.0

Re: [PATCH 6/8] passt, util: Move opening of PID file to its own function

[Richard W.M. Jones]

On Wed, May 22, 2024 at 10:59:09PM +0200, Stefano Brivio wrote:
> We won't call it from main() any longer: move it.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  passt.c | 11 ++---------
>  util.c  | 22 ++++++++++++++++++++++
>  util.h  |  1 +
>  3 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/passt.c b/passt.c
> index fb9773d..e2446fc 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -199,7 +199,7 @@ void exit_handler(int signal)
>   */
>  int main(int argc, char **argv)
>  {
> -	int nfds, i, devnull_fd = -1, pidfile_fd = -1;
> +	int nfds, i, devnull_fd = -1, pidfile_fd;
>  	struct epoll_event events[EPOLL_EVENTS];
>  	char *log_name, argv0[PATH_MAX], *name;
>  	struct ctx c = { 0 };
> @@ -299,14 +299,7 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	if (*c.pid_file) {
> -		if ((pidfile_fd = open(c.pid_file,
> -				       O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
> -				       S_IRUSR | S_IWUSR)) < 0) {
> -			perror("PID file open");
> -			exit(EXIT_FAILURE);
> -		}
> -	}
> +	pidfile_fd = pidfile_open(c.pid_file);
>  
>  	if (isolate_prefork(&c))
>  		die("Failed to sandbox process, exiting");
> diff --git a/util.c b/util.c
> index 18c04ba..cf5a62b 100644
> --- a/util.c
> +++ b/util.c
> @@ -402,6 +402,28 @@ void pidfile_write(int fd, pid_t pid)
>  	close(fd);
>  }
>  
> +/**
> + * pidfile_open() - Open PID file if needed
> + * @path:	Path for PID file, empty string if no PID file is requested
> + *
> + * Return: descriptor for PID file, -1 if path is NULL, won't return on failure
> + */
> +int pidfile_open(const char *path)
> +{
> +	int fd;
> +
> +	if (!*path)
> +		return -1;
> +
> +	if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
> +			     S_IRUSR | S_IWUSR)) < 0) {
> +		perror("PID file open");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	return fd;
> +}
> +
>  /**
>   * __daemon() - daemon()-like function writing PID file before parent exits
>   * @pidfile_fd:	Open PID file descriptor
> diff --git a/util.h b/util.h
> index f811975..8a430ca 100644
> --- a/util.h
> +++ b/util.h
> @@ -156,6 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
>  void ns_enter(const struct ctx *c);
>  bool ns_is_init(void);
>  int open_in_ns(const struct ctx *c, const char *path, int flags);
> +int pidfile_open(const char *path);
>  void pidfile_write(int fd, pid_t pid);
>  int __daemon(int pidfile_fd, int devnull_fd);
>  int fls(unsigned long x);

Trivial refactoring:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Re: [PATCH 6/8] passt, util: Move opening of PID file to its own function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2770 bytes --]

On Wed, May 22, 2024 at 10:59:09PM +0200, Stefano Brivio wrote:
> We won't call it from main() any longer: move it.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  passt.c | 11 ++---------
>  util.c  | 22 ++++++++++++++++++++++
>  util.h  |  1 +
>  3 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/passt.c b/passt.c
> index fb9773d..e2446fc 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -199,7 +199,7 @@ void exit_handler(int signal)
>   */
>  int main(int argc, char **argv)
>  {
> -	int nfds, i, devnull_fd = -1, pidfile_fd = -1;
> +	int nfds, i, devnull_fd = -1, pidfile_fd;
>  	struct epoll_event events[EPOLL_EVENTS];
>  	char *log_name, argv0[PATH_MAX], *name;
>  	struct ctx c = { 0 };
> @@ -299,14 +299,7 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	if (*c.pid_file) {
> -		if ((pidfile_fd = open(c.pid_file,
> -				       O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
> -				       S_IRUSR | S_IWUSR)) < 0) {
> -			perror("PID file open");
> -			exit(EXIT_FAILURE);
> -		}
> -	}
> +	pidfile_fd = pidfile_open(c.pid_file);
>  
>  	if (isolate_prefork(&c))
>  		die("Failed to sandbox process, exiting");
> diff --git a/util.c b/util.c
> index 18c04ba..cf5a62b 100644
> --- a/util.c
> +++ b/util.c
> @@ -402,6 +402,28 @@ void pidfile_write(int fd, pid_t pid)
>  	close(fd);
>  }
>  
> +/**
> + * pidfile_open() - Open PID file if needed
> + * @path:	Path for PID file, empty string if no PID file is requested
> + *
> + * Return: descriptor for PID file, -1 if path is NULL, won't return on failure
> + */
> +int pidfile_open(const char *path)
> +{
> +	int fd;
> +
> +	if (!*path)
> +		return -1;
> +
> +	if ((fd = open(path, O_CREAT | O_TRUNC | O_WRONLY | O_CLOEXEC,
> +			     S_IRUSR | S_IWUSR)) < 0) {
> +		perror("PID file open");
> +		exit(EXIT_FAILURE);
> +	}
> +
> +	return fd;
> +}
> +
>  /**
>   * __daemon() - daemon()-like function writing PID file before parent exits
>   * @pidfile_fd:	Open PID file descriptor
> diff --git a/util.h b/util.h
> index f811975..8a430ca 100644
> --- a/util.h
> +++ b/util.h
> @@ -156,6 +156,7 @@ char *line_read(char *buf, size_t len, int fd);
>  void ns_enter(const struct ctx *c);
>  bool ns_is_init(void);
>  int open_in_ns(const struct ctx *c, const char *path, int flags);
> +int pidfile_open(const char *path);
>  void pidfile_write(int fd, pid_t pid);
>  int __daemon(int pidfile_fd, int devnull_fd);
>  int fls(unsigned long x);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[Stefano Brivio]

Otherwise, if the user runs us as root, and gives us paths that are
only accessible by root, we'll fail to open them, which might in turn
encourage users to change permissions or ownerships: definitely a bad
idea in terms of security.

Reported-by: Minxi Hou <mhou@redhat.com>
Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 conf.c  | 17 ++++++++++++++++-
 passt.c | 10 ++++------
 passt.h |  4 ++++
 tap.c   |  7 +++----
 tap.h   |  1 +
 5 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/conf.c b/conf.c
index 2e0d909..f62a5eb 100644
--- a/conf.c
+++ b/conf.c
@@ -38,6 +38,7 @@
 #include "ip.h"
 #include "passt.h"
 #include "netlink.h"
+#include "tap.h"
 #include "udp.h"
 #include "tcp.h"
 #include "pasta.h"
@@ -1093,7 +1094,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
 		return;
 
 	/* ...otherwise use nobody:nobody */
-	warn("Started as root. Changing to nobody...");
+	warn("Started as root, will change to nobody.");
 	{
 #ifndef GLIBC_NO_STATIC_NSS
 		const struct passwd *pw;
@@ -1113,6 +1114,18 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
 	}
 }
 
+/**
+ * conf_open_files() - Open files as requested by configuration
+ * @c:		Execution context
+ */
+static void conf_open_files(struct ctx *c)
+{
+	if (c->mode == MODE_PASST && c->fd_tap == -1)
+		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
+
+	c->pidfile_fd = pidfile_open(c->pid_file);
+}
+
 /**
  * conf() - Process command-line arguments and set configuration
  * @c:		Execution context
@@ -1712,6 +1725,8 @@ void conf(struct ctx *c, int argc, char **argv)
 	else if (optind != argc)
 		die("Extra non-option argument: %s", argv[optind]);
 
+	conf_open_files(c);	/* Before any possible setuid() / setgid() */
+
 	isolate_user(uid, gid, !netns_only, userns, c->mode);
 
 	if (c->pasta_conf_ns)
diff --git a/passt.c b/passt.c
index e2446fc..a8c4cd3 100644
--- a/passt.c
+++ b/passt.c
@@ -199,9 +199,9 @@ void exit_handler(int signal)
  */
 int main(int argc, char **argv)
 {
-	int nfds, i, devnull_fd = -1, pidfile_fd;
 	struct epoll_event events[EPOLL_EVENTS];
 	char *log_name, argv0[PATH_MAX], *name;
+	int nfds, i, devnull_fd = -1;
 	struct ctx c = { 0 };
 	struct rlimit limit;
 	struct timespec now;
@@ -211,7 +211,7 @@ int main(int argc, char **argv)
 
 	isolate_initial();
 
-	c.pasta_netns_fd = c.fd_tap = -1;
+	c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1;
 
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
@@ -299,8 +299,6 @@ int main(int argc, char **argv)
 		}
 	}
 
-	pidfile_fd = pidfile_open(c.pid_file);
-
 	if (isolate_prefork(&c))
 		die("Failed to sandbox process, exiting");
 
@@ -308,9 +306,9 @@ int main(int argc, char **argv)
 		__openlog(log_name, 0, LOG_DAEMON);
 
 	if (!c.foreground)
-		__daemon(pidfile_fd, devnull_fd);
+		__daemon(c.pidfile_fd, devnull_fd);
 	else
-		pidfile_write(pidfile_fd, getpid());
+		pidfile_write(c.pidfile_fd, getpid());
 
 	if (pasta_child_pid)
 		kill(pasta_child_pid, SIGUSR1);
diff --git a/passt.h b/passt.h
index bc58d64..3e50612 100644
--- a/passt.h
+++ b/passt.h
@@ -185,6 +185,7 @@ struct ip6_ctx {
  * @sock_path:		Path for UNIX domain socket
  * @pcap:		Path for packet capture file
  * @pid_file:		Path to PID file, empty string if not configured
+ * @pidfile_fd:		File descriptor for PID file, -1 if none
  * @pasta_netns_fd:	File descriptor for network namespace in pasta mode
  * @no_netns_quit:	In pasta mode, don't exit if fs-bound namespace is gone
  * @netns_base:		Base name for fs-bound namespace, if any, in pasta mode
@@ -234,7 +235,10 @@ struct ctx {
 	int nofile;
 	char sock_path[UNIX_PATH_MAX];
 	char pcap[PATH_MAX];
+
 	char pid_file[PATH_MAX];
+	int pidfile_fd;
+
 	int one_off;
 
 	int pasta_netns_fd;
diff --git a/tap.c b/tap.c
index c9f580e..2ea0849 100644
--- a/tap.c
+++ b/tap.c
@@ -1100,7 +1100,7 @@ restart:
  *
  * Return: socket descriptor on success, won't return on failure
  */
-static int tap_sock_unix_open(char *sock_path)
+int tap_sock_unix_open(char *sock_path)
 {
 	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
 	struct sockaddr_un addr = {
@@ -1144,7 +1144,7 @@ static int tap_sock_unix_open(char *sock_path)
 	if (i == UNIX_SOCK_MAX)
 		die("UNIX socket bind: %s", strerror(errno));
 
-	info("UNIX domain socket bound at %s\n", addr.sun_path);
+	info("UNIX domain socket bound at %s", addr.sun_path);
 	if (!*sock_path)
 		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
 
@@ -1167,7 +1167,7 @@ static void tap_sock_unix_init(struct ctx *c)
 	ev.data.u64 = ref.u64;
 	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
 
-	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
+	info("\nYou can now start qemu (>= 7.2, with commit 13c6be96618c):");
 	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
 	     c->sock_path);
 	info("or qrap, for earlier qemu versions:");
@@ -1318,7 +1318,6 @@ void tap_sock_init(struct ctx *c)
 	}
 
 	if (c->mode == MODE_PASST) {
-		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
 		tap_sock_unix_init(c);
 
 		/* In passt mode, we don't know the guest's MAC address until it
diff --git a/tap.h b/tap.h
index d146d2f..2285a87 100644
--- a/tap.h
+++ b/tap.h
@@ -68,6 +68,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
 		       const struct timespec *now);
 void tap_handler_passt(struct ctx *c, uint32_t events,
 		       const struct timespec *now);
+int tap_sock_unix_open(char *sock_path);
 void tap_sock_init(struct ctx *c);
 
 #endif /* TAP_H */
-- 
@@ -68,6 +68,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
 		       const struct timespec *now);
 void tap_handler_passt(struct ctx *c, uint32_t events,
 		       const struct timespec *now);
+int tap_sock_unix_open(char *sock_path);
 void tap_sock_init(struct ctx *c);
 
 #endif /* TAP_H */
-- 
2.43.0

Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[Richard W.M. Jones]

On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:
> Otherwise, if the user runs us as root, and gives us paths that are
> only accessible by root, we'll fail to open them, which might in turn
> encourage users to change permissions or ownerships: definitely a bad
> idea in terms of security.
> 
> Reported-by: Minxi Hou <mhou@redhat.com>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  conf.c  | 17 ++++++++++++++++-
>  passt.c | 10 ++++------
>  passt.h |  4 ++++
>  tap.c   |  7 +++----
>  tap.h   |  1 +
>  5 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 2e0d909..f62a5eb 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -38,6 +38,7 @@
>  #include "ip.h"
>  #include "passt.h"
>  #include "netlink.h"
> +#include "tap.h"
>  #include "udp.h"
>  #include "tcp.h"
>  #include "pasta.h"
> @@ -1093,7 +1094,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  		return;
>  
>  	/* ...otherwise use nobody:nobody */
> -	warn("Started as root. Changing to nobody...");
> +	warn("Started as root, will change to nobody.");
>  	{
>  #ifndef GLIBC_NO_STATIC_NSS
>  		const struct passwd *pw;
> @@ -1113,6 +1114,18 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  	}
>  }
>  
> +/**
> + * conf_open_files() - Open files as requested by configuration
> + * @c:		Execution context
> + */
> +static void conf_open_files(struct ctx *c)
> +{
> +	if (c->mode == MODE_PASST && c->fd_tap == -1)
> +		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
> +
> +	c->pidfile_fd = pidfile_open(c->pid_file);
> +}
> +
>  /**
>   * conf() - Process command-line arguments and set configuration
>   * @c:		Execution context
> @@ -1712,6 +1725,8 @@ void conf(struct ctx *c, int argc, char **argv)
>  	else if (optind != argc)
>  		die("Extra non-option argument: %s", argv[optind]);
>  
> +	conf_open_files(c);	/* Before any possible setuid() / setgid() */
> +
>  	isolate_user(uid, gid, !netns_only, userns, c->mode);
>  
>  	if (c->pasta_conf_ns)
> diff --git a/passt.c b/passt.c
> index e2446fc..a8c4cd3 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -199,9 +199,9 @@ void exit_handler(int signal)
>   */
>  int main(int argc, char **argv)
>  {
> -	int nfds, i, devnull_fd = -1, pidfile_fd;
>  	struct epoll_event events[EPOLL_EVENTS];
>  	char *log_name, argv0[PATH_MAX], *name;
> +	int nfds, i, devnull_fd = -1;
>  	struct ctx c = { 0 };
>  	struct rlimit limit;
>  	struct timespec now;
> @@ -211,7 +211,7 @@ int main(int argc, char **argv)
>  
>  	isolate_initial();
>  
> -	c.pasta_netns_fd = c.fd_tap = -1;
> +	c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1;
>  
>  	sigemptyset(&sa.sa_mask);
>  	sa.sa_flags = 0;
> @@ -299,8 +299,6 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	pidfile_fd = pidfile_open(c.pid_file);
> -
>  	if (isolate_prefork(&c))
>  		die("Failed to sandbox process, exiting");
>  
> @@ -308,9 +306,9 @@ int main(int argc, char **argv)
>  		__openlog(log_name, 0, LOG_DAEMON);
>  
>  	if (!c.foreground)
> -		__daemon(pidfile_fd, devnull_fd);
> +		__daemon(c.pidfile_fd, devnull_fd);
>  	else
> -		pidfile_write(pidfile_fd, getpid());
> +		pidfile_write(c.pidfile_fd, getpid());
>  
>  	if (pasta_child_pid)
>  		kill(pasta_child_pid, SIGUSR1);
> diff --git a/passt.h b/passt.h
> index bc58d64..3e50612 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -185,6 +185,7 @@ struct ip6_ctx {
>   * @sock_path:		Path for UNIX domain socket
>   * @pcap:		Path for packet capture file
>   * @pid_file:		Path to PID file, empty string if not configured
> + * @pidfile_fd:		File descriptor for PID file, -1 if none
>   * @pasta_netns_fd:	File descriptor for network namespace in pasta mode
>   * @no_netns_quit:	In pasta mode, don't exit if fs-bound namespace is gone
>   * @netns_base:		Base name for fs-bound namespace, if any, in pasta mode
> @@ -234,7 +235,10 @@ struct ctx {
>  	int nofile;
>  	char sock_path[UNIX_PATH_MAX];
>  	char pcap[PATH_MAX];
> +
>  	char pid_file[PATH_MAX];
> +	int pidfile_fd;
> +
>  	int one_off;
>  
>  	int pasta_netns_fd;
> diff --git a/tap.c b/tap.c
> index c9f580e..2ea0849 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1100,7 +1100,7 @@ restart:
>   *
>   * Return: socket descriptor on success, won't return on failure
>   */
> -static int tap_sock_unix_open(char *sock_path)
> +int tap_sock_unix_open(char *sock_path)
>  {
>  	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
>  	struct sockaddr_un addr = {
> @@ -1144,7 +1144,7 @@ static int tap_sock_unix_open(char *sock_path)
>  	if (i == UNIX_SOCK_MAX)
>  		die("UNIX socket bind: %s", strerror(errno));
>  
> -	info("UNIX domain socket bound at %s\n", addr.sun_path);
> +	info("UNIX domain socket bound at %s", addr.sun_path);
>  	if (!*sock_path)
>  		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
>  
> @@ -1167,7 +1167,7 @@ static void tap_sock_unix_init(struct ctx *c)
>  	ev.data.u64 = ref.u64;
>  	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
>  
> -	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
> +	info("\nYou can now start qemu (>= 7.2, with commit 13c6be96618c):");
>  	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
>  	     c->sock_path);
>  	info("or qrap, for earlier qemu versions:");
> @@ -1318,7 +1318,6 @@ void tap_sock_init(struct ctx *c)
>  	}
>  
>  	if (c->mode == MODE_PASST) {
> -		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
>  		tap_sock_unix_init(c);
>  
>  		/* In passt mode, we don't know the guest's MAC address until it
> diff --git a/tap.h b/tap.h
> index d146d2f..2285a87 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -68,6 +68,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
>  		       const struct timespec *now);
>  void tap_handler_passt(struct ctx *c, uint32_t events,
>  		       const struct timespec *now);
> +int tap_sock_unix_open(char *sock_path);
>  void tap_sock_init(struct ctx *c);
>  
>  #endif /* TAP_H */

Seems sensible.  The code to open the pidfile definitely moves to the
new function and the new function is called just before passt isolates
itself in the new namespace.

Acked-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6992 bytes --]

On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:
> Otherwise, if the user runs us as root, and gives us paths that are
> only accessible by root, we'll fail to open them, which might in turn
> encourage users to change permissions or ownerships: definitely a bad
> idea in terms of security.
> 
> Reported-by: Minxi Hou <mhou@redhat.com>
> Reported-by: Richard W.M. Jones <rjones@redhat.com>
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Looking at this I did notice a pre-existing, well, maybe not bug
exactly, but possibly surprising behaviour, which makes me a
bit more nervous now that we can invoke it as root.

tap_sock_unix_open() will silently truncate the given socket path to
the maximum length for a Unix socket.  Which means we could bind(),
but also unlink() a path that's not exactly the same as the one the
one the user requested.  I don't immediately see a way to exploit
that, but it's the sort of thing that makes me nervous.  I think we
should instead outright fail if the given socket path is too long.

> ---
>  conf.c  | 17 ++++++++++++++++-
>  passt.c | 10 ++++------
>  passt.h |  4 ++++
>  tap.c   |  7 +++----
>  tap.h   |  1 +
>  5 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 2e0d909..f62a5eb 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -38,6 +38,7 @@
>  #include "ip.h"
>  #include "passt.h"
>  #include "netlink.h"
> +#include "tap.h"
>  #include "udp.h"
>  #include "tcp.h"
>  #include "pasta.h"
> @@ -1093,7 +1094,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  		return;
>  
>  	/* ...otherwise use nobody:nobody */
> -	warn("Started as root. Changing to nobody...");
> +	warn("Started as root, will change to nobody.");
>  	{
>  #ifndef GLIBC_NO_STATIC_NSS
>  		const struct passwd *pw;
> @@ -1113,6 +1114,18 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
>  	}
>  }
>  
> +/**
> + * conf_open_files() - Open files as requested by configuration
> + * @c:		Execution context
> + */
> +static void conf_open_files(struct ctx *c)
> +{
> +	if (c->mode == MODE_PASST && c->fd_tap == -1)
> +		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
> +
> +	c->pidfile_fd = pidfile_open(c->pid_file);
> +}
> +
>  /**
>   * conf() - Process command-line arguments and set configuration
>   * @c:		Execution context
> @@ -1712,6 +1725,8 @@ void conf(struct ctx *c, int argc, char **argv)
>  	else if (optind != argc)
>  		die("Extra non-option argument: %s", argv[optind]);
>  
> +	conf_open_files(c);	/* Before any possible setuid() / setgid() */
> +
>  	isolate_user(uid, gid, !netns_only, userns, c->mode);
>  
>  	if (c->pasta_conf_ns)
> diff --git a/passt.c b/passt.c
> index e2446fc..a8c4cd3 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -199,9 +199,9 @@ void exit_handler(int signal)
>   */
>  int main(int argc, char **argv)
>  {
> -	int nfds, i, devnull_fd = -1, pidfile_fd;
>  	struct epoll_event events[EPOLL_EVENTS];
>  	char *log_name, argv0[PATH_MAX], *name;
> +	int nfds, i, devnull_fd = -1;
>  	struct ctx c = { 0 };
>  	struct rlimit limit;
>  	struct timespec now;
> @@ -211,7 +211,7 @@ int main(int argc, char **argv)
>  
>  	isolate_initial();
>  
> -	c.pasta_netns_fd = c.fd_tap = -1;
> +	c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1;
>  
>  	sigemptyset(&sa.sa_mask);
>  	sa.sa_flags = 0;
> @@ -299,8 +299,6 @@ int main(int argc, char **argv)
>  		}
>  	}
>  
> -	pidfile_fd = pidfile_open(c.pid_file);
> -
>  	if (isolate_prefork(&c))
>  		die("Failed to sandbox process, exiting");
>  
> @@ -308,9 +306,9 @@ int main(int argc, char **argv)
>  		__openlog(log_name, 0, LOG_DAEMON);
>  
>  	if (!c.foreground)
> -		__daemon(pidfile_fd, devnull_fd);
> +		__daemon(c.pidfile_fd, devnull_fd);
>  	else
> -		pidfile_write(pidfile_fd, getpid());
> +		pidfile_write(c.pidfile_fd, getpid());
>  
>  	if (pasta_child_pid)
>  		kill(pasta_child_pid, SIGUSR1);
> diff --git a/passt.h b/passt.h
> index bc58d64..3e50612 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -185,6 +185,7 @@ struct ip6_ctx {
>   * @sock_path:		Path for UNIX domain socket
>   * @pcap:		Path for packet capture file
>   * @pid_file:		Path to PID file, empty string if not configured
> + * @pidfile_fd:		File descriptor for PID file, -1 if none
>   * @pasta_netns_fd:	File descriptor for network namespace in pasta mode
>   * @no_netns_quit:	In pasta mode, don't exit if fs-bound namespace is gone
>   * @netns_base:		Base name for fs-bound namespace, if any, in pasta mode
> @@ -234,7 +235,10 @@ struct ctx {
>  	int nofile;
>  	char sock_path[UNIX_PATH_MAX];
>  	char pcap[PATH_MAX];
> +
>  	char pid_file[PATH_MAX];
> +	int pidfile_fd;
> +
>  	int one_off;
>  
>  	int pasta_netns_fd;
> diff --git a/tap.c b/tap.c
> index c9f580e..2ea0849 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1100,7 +1100,7 @@ restart:
>   *
>   * Return: socket descriptor on success, won't return on failure
>   */
> -static int tap_sock_unix_open(char *sock_path)
> +int tap_sock_unix_open(char *sock_path)
>  {
>  	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
>  	struct sockaddr_un addr = {
> @@ -1144,7 +1144,7 @@ static int tap_sock_unix_open(char *sock_path)
>  	if (i == UNIX_SOCK_MAX)
>  		die("UNIX socket bind: %s", strerror(errno));
>  
> -	info("UNIX domain socket bound at %s\n", addr.sun_path);
> +	info("UNIX domain socket bound at %s", addr.sun_path);
>  	if (!*sock_path)
>  		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
>  
> @@ -1167,7 +1167,7 @@ static void tap_sock_unix_init(struct ctx *c)
>  	ev.data.u64 = ref.u64;
>  	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
>  
> -	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
> +	info("\nYou can now start qemu (>= 7.2, with commit 13c6be96618c):");
>  	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
>  	     c->sock_path);
>  	info("or qrap, for earlier qemu versions:");
> @@ -1318,7 +1318,6 @@ void tap_sock_init(struct ctx *c)
>  	}
>  
>  	if (c->mode == MODE_PASST) {
> -		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
>  		tap_sock_unix_init(c);
>  
>  		/* In passt mode, we don't know the guest's MAC address until it
> diff --git a/tap.h b/tap.h
> index d146d2f..2285a87 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -68,6 +68,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
>  		       const struct timespec *now);
>  void tap_handler_passt(struct ctx *c, uint32_t events,
>  		       const struct timespec *now);
> +int tap_sock_unix_open(char *sock_path);
>  void tap_sock_init(struct ctx *c);
>  
>  #endif /* TAP_H */

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[Richard W.M. Jones]

On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:
> On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:
> > Otherwise, if the user runs us as root, and gives us paths that are
> > only accessible by root, we'll fail to open them, which might in turn
> > encourage users to change permissions or ownerships: definitely a bad
> > idea in terms of security.
> > 
> > Reported-by: Minxi Hou <mhou@redhat.com>
> > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> 
> Looking at this I did notice a pre-existing, well, maybe not bug
> exactly, but possibly surprising behaviour, which makes me a
> bit more nervous now that we can invoke it as root.
> 
> tap_sock_unix_open() will silently truncate the given socket path to
> the maximum length for a Unix socket.  Which means we could bind(),
> but also unlink() a path that's not exactly the same as the one the
> one the user requested.  I don't immediately see a way to exploit
> that, but it's the sort of thing that makes me nervous.  I think we
> should instead outright fail if the given socket path is too long.

Yes, agreed.

It seems as if the latest passt code still does this.  Do you want me
to open a bug about it?

Rich.

> > ---
> >  conf.c  | 17 ++++++++++++++++-
> >  passt.c | 10 ++++------
> >  passt.h |  4 ++++
> >  tap.c   |  7 +++----
> >  tap.h   |  1 +
> >  5 files changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 2e0d909..f62a5eb 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -38,6 +38,7 @@
> >  #include "ip.h"
> >  #include "passt.h"
> >  #include "netlink.h"
> > +#include "tap.h"
> >  #include "udp.h"
> >  #include "tcp.h"
> >  #include "pasta.h"
> > @@ -1093,7 +1094,7 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
> >  		return;
> >  
> >  	/* ...otherwise use nobody:nobody */
> > -	warn("Started as root. Changing to nobody...");
> > +	warn("Started as root, will change to nobody.");
> >  	{
> >  #ifndef GLIBC_NO_STATIC_NSS
> >  		const struct passwd *pw;
> > @@ -1113,6 +1114,18 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid)
> >  	}
> >  }
> >  
> > +/**
> > + * conf_open_files() - Open files as requested by configuration
> > + * @c:		Execution context
> > + */
> > +static void conf_open_files(struct ctx *c)
> > +{
> > +	if (c->mode == MODE_PASST && c->fd_tap == -1)
> > +		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
> > +
> > +	c->pidfile_fd = pidfile_open(c->pid_file);
> > +}
> > +
> >  /**
> >   * conf() - Process command-line arguments and set configuration
> >   * @c:		Execution context
> > @@ -1712,6 +1725,8 @@ void conf(struct ctx *c, int argc, char **argv)
> >  	else if (optind != argc)
> >  		die("Extra non-option argument: %s", argv[optind]);
> >  
> > +	conf_open_files(c);	/* Before any possible setuid() / setgid() */
> > +
> >  	isolate_user(uid, gid, !netns_only, userns, c->mode);
> >  
> >  	if (c->pasta_conf_ns)
> > diff --git a/passt.c b/passt.c
> > index e2446fc..a8c4cd3 100644
> > --- a/passt.c
> > +++ b/passt.c
> > @@ -199,9 +199,9 @@ void exit_handler(int signal)
> >   */
> >  int main(int argc, char **argv)
> >  {
> > -	int nfds, i, devnull_fd = -1, pidfile_fd;
> >  	struct epoll_event events[EPOLL_EVENTS];
> >  	char *log_name, argv0[PATH_MAX], *name;
> > +	int nfds, i, devnull_fd = -1;
> >  	struct ctx c = { 0 };
> >  	struct rlimit limit;
> >  	struct timespec now;
> > @@ -211,7 +211,7 @@ int main(int argc, char **argv)
> >  
> >  	isolate_initial();
> >  
> > -	c.pasta_netns_fd = c.fd_tap = -1;
> > +	c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1;
> >  
> >  	sigemptyset(&sa.sa_mask);
> >  	sa.sa_flags = 0;
> > @@ -299,8 +299,6 @@ int main(int argc, char **argv)
> >  		}
> >  	}
> >  
> > -	pidfile_fd = pidfile_open(c.pid_file);
> > -
> >  	if (isolate_prefork(&c))
> >  		die("Failed to sandbox process, exiting");
> >  
> > @@ -308,9 +306,9 @@ int main(int argc, char **argv)
> >  		__openlog(log_name, 0, LOG_DAEMON);
> >  
> >  	if (!c.foreground)
> > -		__daemon(pidfile_fd, devnull_fd);
> > +		__daemon(c.pidfile_fd, devnull_fd);
> >  	else
> > -		pidfile_write(pidfile_fd, getpid());
> > +		pidfile_write(c.pidfile_fd, getpid());
> >  
> >  	if (pasta_child_pid)
> >  		kill(pasta_child_pid, SIGUSR1);
> > diff --git a/passt.h b/passt.h
> > index bc58d64..3e50612 100644
> > --- a/passt.h
> > +++ b/passt.h
> > @@ -185,6 +185,7 @@ struct ip6_ctx {
> >   * @sock_path:		Path for UNIX domain socket
> >   * @pcap:		Path for packet capture file
> >   * @pid_file:		Path to PID file, empty string if not configured
> > + * @pidfile_fd:		File descriptor for PID file, -1 if none
> >   * @pasta_netns_fd:	File descriptor for network namespace in pasta mode
> >   * @no_netns_quit:	In pasta mode, don't exit if fs-bound namespace is gone
> >   * @netns_base:		Base name for fs-bound namespace, if any, in pasta mode
> > @@ -234,7 +235,10 @@ struct ctx {
> >  	int nofile;
> >  	char sock_path[UNIX_PATH_MAX];
> >  	char pcap[PATH_MAX];
> > +
> >  	char pid_file[PATH_MAX];
> > +	int pidfile_fd;
> > +
> >  	int one_off;
> >  
> >  	int pasta_netns_fd;
> > diff --git a/tap.c b/tap.c
> > index c9f580e..2ea0849 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -1100,7 +1100,7 @@ restart:
> >   *
> >   * Return: socket descriptor on success, won't return on failure
> >   */
> > -static int tap_sock_unix_open(char *sock_path)
> > +int tap_sock_unix_open(char *sock_path)
> >  {
> >  	int fd = socket(AF_UNIX, SOCK_STREAM, 0);
> >  	struct sockaddr_un addr = {
> > @@ -1144,7 +1144,7 @@ static int tap_sock_unix_open(char *sock_path)
> >  	if (i == UNIX_SOCK_MAX)
> >  		die("UNIX socket bind: %s", strerror(errno));
> >  
> > -	info("UNIX domain socket bound at %s\n", addr.sun_path);
> > +	info("UNIX domain socket bound at %s", addr.sun_path);
> >  	if (!*sock_path)
> >  		memcpy(sock_path, addr.sun_path, UNIX_PATH_MAX);
> >  
> > @@ -1167,7 +1167,7 @@ static void tap_sock_unix_init(struct ctx *c)
> >  	ev.data.u64 = ref.u64;
> >  	epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap_listen, &ev);
> >  
> > -	info("You can now start qemu (>= 7.2, with commit 13c6be96618c):");
> > +	info("\nYou can now start qemu (>= 7.2, with commit 13c6be96618c):");
> >  	info("    kvm ... -device virtio-net-pci,netdev=s -netdev stream,id=s,server=off,addr.type=unix,addr.path=%s",
> >  	     c->sock_path);
> >  	info("or qrap, for earlier qemu versions:");
> > @@ -1318,7 +1318,6 @@ void tap_sock_init(struct ctx *c)
> >  	}
> >  
> >  	if (c->mode == MODE_PASST) {
> > -		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
> >  		tap_sock_unix_init(c);
> >  
> >  		/* In passt mode, we don't know the guest's MAC address until it
> > diff --git a/tap.h b/tap.h
> > index d146d2f..2285a87 100644
> > --- a/tap.h
> > +++ b/tap.h
> > @@ -68,6 +68,7 @@ void tap_handler_pasta(struct ctx *c, uint32_t events,
> >  		       const struct timespec *now);
> >  void tap_handler_passt(struct ctx *c, uint32_t events,
> >  		       const struct timespec *now);
> > +int tap_sock_unix_open(char *sock_path);
> >  void tap_sock_init(struct ctx *c);
> >  
> >  #endif /* TAP_H */
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson



-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
nbdkit - Flexible, fast NBD server with plugins
https://gitlab.com/nbdkit/nbdkit

Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[Stefano Brivio]

On Thu, 20 Jun 2024 12:30:54 +0100
"Richard W.M. Jones" <rjones@redhat.com> wrote:

> On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:
> > On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:  
> > > Otherwise, if the user runs us as root, and gives us paths that are
> > > only accessible by root, we'll fail to open them, which might in turn
> > > encourage users to change permissions or ownerships: definitely a bad
> > > idea in terms of security.
> > > 
> > > Reported-by: Minxi Hou <mhou@redhat.com>
> > > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > 
> > Looking at this I did notice a pre-existing, well, maybe not bug
> > exactly, but possibly surprising behaviour, which makes me a
> > bit more nervous now that we can invoke it as root.
> > 
> > tap_sock_unix_open() will silently truncate the given socket path to
> > the maximum length for a Unix socket.  Which means we could bind(),
> > but also unlink() a path that's not exactly the same as the one the
> > one the user requested.  I don't immediately see a way to exploit
> > that, but it's the sort of thing that makes me nervous.  I think we
> > should instead outright fail if the given socket path is too long.  
> 
> Yes, agreed.
> 
> It seems as if the latest passt code still does this.  Do you want me
> to open a bug about it?

Yes, please, that, or a patch :)

-- 
Stefano

Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[Richard W.M. Jones]

On Thu, Jun 20, 2024 at 02:12:53PM +0200, Stefano Brivio wrote:
> On Thu, 20 Jun 2024 12:30:54 +0100
> "Richard W.M. Jones" <rjones@redhat.com> wrote:
> 
> > On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:
> > > On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:  
> > > > Otherwise, if the user runs us as root, and gives us paths that are
> > > > only accessible by root, we'll fail to open them, which might in turn
> > > > encourage users to change permissions or ownerships: definitely a bad
> > > > idea in terms of security.
> > > > 
> > > > Reported-by: Minxi Hou <mhou@redhat.com>
> > > > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > > 
> > > Looking at this I did notice a pre-existing, well, maybe not bug
> > > exactly, but possibly surprising behaviour, which makes me a
> > > bit more nervous now that we can invoke it as root.
> > > 
> > > tap_sock_unix_open() will silently truncate the given socket path to
> > > the maximum length for a Unix socket.  Which means we could bind(),
> > > but also unlink() a path that's not exactly the same as the one the
> > > one the user requested.  I don't immediately see a way to exploit
> > > that, but it's the sort of thing that makes me nervous.  I think we
> > > should instead outright fail if the given socket path is too long.  
> > 
> > Yes, agreed.
> > 
> > It seems as if the latest passt code still does this.  Do you want me
> > to open a bug about it?
> 
> Yes, please, that, or a patch :)

While I was testing this, I found we do seem to check it:

https://passt.top/passt/tree/conf.c#n1446

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[Stefano Brivio]

On Thu, 20 Jun 2024 13:47:31 +0100
"Richard W.M. Jones" <rjones@redhat.com> wrote:

> On Thu, Jun 20, 2024 at 02:12:53PM +0200, Stefano Brivio wrote:
> > On Thu, 20 Jun 2024 12:30:54 +0100
> > "Richard W.M. Jones" <rjones@redhat.com> wrote:
> >   
> > > On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:  
> > > > On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:    
> > > > > Otherwise, if the user runs us as root, and gives us paths that are
> > > > > only accessible by root, we'll fail to open them, which might in turn
> > > > > encourage users to change permissions or ownerships: definitely a bad
> > > > > idea in terms of security.
> > > > > 
> > > > > Reported-by: Minxi Hou <mhou@redhat.com>
> > > > > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>    
> > > > 
> > > > Looking at this I did notice a pre-existing, well, maybe not bug
> > > > exactly, but possibly surprising behaviour, which makes me a
> > > > bit more nervous now that we can invoke it as root.
> > > > 
> > > > tap_sock_unix_open() will silently truncate the given socket path to
> > > > the maximum length for a Unix socket.  Which means we could bind(),
> > > > but also unlink() a path that's not exactly the same as the one the
> > > > one the user requested.  I don't immediately see a way to exploit
> > > > that, but it's the sort of thing that makes me nervous.  I think we
> > > > should instead outright fail if the given socket path is too long.    
> > > 
> > > Yes, agreed.
> > > 
> > > It seems as if the latest passt code still does this.  Do you want me
> > > to open a bug about it?  
> > 
> > Yes, please, that, or a patch :)  
> 
> While I was testing this, I found we do seem to check it:
> 
> https://passt.top/passt/tree/conf.c#n1446

Oh, I thought David was referring to the loop in tap_sock_unix_open(),
where we try paths in the form "/tmp/passt_%i.socket". But even there,
we can't exceed UNIX_PATH_MAX.

One minor issue remains, though: in conf(), we refuse paths that are
longer than UNIX_SOCK_MAX (100). That's the maximum index for the
"/tmp/passt_%i.socket", it happens to be a sane value, but we should use
UNIX_PATH_MAX (108) instead. I'll fix it, but wait for David's feedback
first.

-- 
Stefano

Re: [PATCH 7/8] conf, passt, tap: Open socket and PID files before switching UID/GID

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3030 bytes --]

On Thu, Jun 20, 2024 at 04:22:12PM +0200, Stefano Brivio wrote:
> On Thu, 20 Jun 2024 13:47:31 +0100
> "Richard W.M. Jones" <rjones@redhat.com> wrote:
> 
> > On Thu, Jun 20, 2024 at 02:12:53PM +0200, Stefano Brivio wrote:
> > > On Thu, 20 Jun 2024 12:30:54 +0100
> > > "Richard W.M. Jones" <rjones@redhat.com> wrote:
> > >   
> > > > On Wed, May 29, 2024 at 12:35:24PM +1000, David Gibson wrote:  
> > > > > On Wed, May 22, 2024 at 10:59:10PM +0200, Stefano Brivio wrote:    
> > > > > > Otherwise, if the user runs us as root, and gives us paths that are
> > > > > > only accessible by root, we'll fail to open them, which might in turn
> > > > > > encourage users to change permissions or ownerships: definitely a bad
> > > > > > idea in terms of security.
> > > > > > 
> > > > > > Reported-by: Minxi Hou <mhou@redhat.com>
> > > > > > Reported-by: Richard W.M. Jones <rjones@redhat.com>
> > > > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>    
> > > > > 
> > > > > Looking at this I did notice a pre-existing, well, maybe not bug
> > > > > exactly, but possibly surprising behaviour, which makes me a
> > > > > bit more nervous now that we can invoke it as root.
> > > > > 
> > > > > tap_sock_unix_open() will silently truncate the given socket path to
> > > > > the maximum length for a Unix socket.  Which means we could bind(),
> > > > > but also unlink() a path that's not exactly the same as the one the
> > > > > one the user requested.  I don't immediately see a way to exploit
> > > > > that, but it's the sort of thing that makes me nervous.  I think we
> > > > > should instead outright fail if the given socket path is too long.    
> > > > 
> > > > Yes, agreed.
> > > > 
> > > > It seems as if the latest passt code still does this.  Do you want me
> > > > to open a bug about it?  
> > > 
> > > Yes, please, that, or a patch :)  
> > 
> > While I was testing this, I found we do seem to check it:
> > 
> > https://passt.top/passt/tree/conf.c#n1446
> 
> Oh, I thought David was referring to the loop in tap_sock_unix_open(),
> where we try paths in the form "/tmp/passt_%i.socket". But even there,
> we can't exceed UNIX_PATH_MAX.

Actually I was referring to the memcpy() from sock_path to path in
tap_sock_unix_open().  I hadn't thought to check if we'd previously
verified the length of sock_path.  So, yeah we seem to be ok here
(although it's not obvious that's the case from within the code of
tap_sock_unix_open()).

> One minor issue remains, though: in conf(), we refuse paths that are
> longer than UNIX_SOCK_MAX (100). That's the maximum index for the
> "/tmp/passt_%i.socket", it happens to be a sane value, but we should use
> UNIX_PATH_MAX (108) instead. I'll fix it, but wait for David's feedback
> first.

Well, apart from that, yes.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 8/8] conf, passt.h: Rename pid_file in struct ctx to pidfile

[Stefano Brivio]

We have pidfile_fd now, pid_file_fd would be quite ugly.

Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 conf.c  | 8 ++++----
 passt.h | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/conf.c b/conf.c
index f62a5eb..50383a3 100644
--- a/conf.c
+++ b/conf.c
@@ -1123,7 +1123,7 @@ static void conf_open_files(struct ctx *c)
 	if (c->mode == MODE_PASST && c->fd_tap == -1)
 		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
 
-	c->pidfile_fd = pidfile_open(c->pid_file);
+	c->pidfile_fd = pidfile_open(c->pidfile);
 }
 
 /**
@@ -1456,12 +1456,12 @@ void conf(struct ctx *c, int argc, char **argv)
 
 			break;
 		case 'P':
-			if (*c->pid_file)
+			if (*c->pidfile)
 				die("Multiple --pid options given");
 
-			ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s",
+			ret = snprintf(c->pidfile, sizeof(c->pidfile), "%s",
 				       optarg);
-			if (ret <= 0 || ret >= (int)sizeof(c->pid_file))
+			if (ret <= 0 || ret >= (int)sizeof(c->pidfile))
 				die("Invalid PID file: %s", optarg);
 
 			break;
diff --git a/passt.h b/passt.h
index 3e50612..46d073a 100644
--- a/passt.h
+++ b/passt.h
@@ -184,7 +184,7 @@ struct ip6_ctx {
  * @nofile:		Maximum number of open files (ulimit -n)
  * @sock_path:		Path for UNIX domain socket
  * @pcap:		Path for packet capture file
- * @pid_file:		Path to PID file, empty string if not configured
+ * @pidfile:		Path to PID file, empty string if not configured
  * @pidfile_fd:		File descriptor for PID file, -1 if none
  * @pasta_netns_fd:	File descriptor for network namespace in pasta mode
  * @no_netns_quit:	In pasta mode, don't exit if fs-bound namespace is gone
@@ -236,7 +236,7 @@ struct ctx {
 	char sock_path[UNIX_PATH_MAX];
 	char pcap[PATH_MAX];
 
-	char pid_file[PATH_MAX];
+	char pidfile[PATH_MAX];
 	int pidfile_fd;
 
 	int one_off;
-- 
@@ -184,7 +184,7 @@ struct ip6_ctx {
  * @nofile:		Maximum number of open files (ulimit -n)
  * @sock_path:		Path for UNIX domain socket
  * @pcap:		Path for packet capture file
- * @pid_file:		Path to PID file, empty string if not configured
+ * @pidfile:		Path to PID file, empty string if not configured
  * @pidfile_fd:		File descriptor for PID file, -1 if none
  * @pasta_netns_fd:	File descriptor for network namespace in pasta mode
  * @no_netns_quit:	In pasta mode, don't exit if fs-bound namespace is gone
@@ -236,7 +236,7 @@ struct ctx {
 	char sock_path[UNIX_PATH_MAX];
 	char pcap[PATH_MAX];
 
-	char pid_file[PATH_MAX];
+	char pidfile[PATH_MAX];
 	int pidfile_fd;
 
 	int one_off;
-- 
2.43.0

Re: [PATCH 8/8] conf, passt.h: Rename pid_file in struct ctx to pidfile

[Richard W.M. Jones]

On Wed, May 22, 2024 at 10:59:11PM +0200, Stefano Brivio wrote:
> We have pidfile_fd now, pid_file_fd would be quite ugly.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  conf.c  | 8 ++++----
>  passt.h | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index f62a5eb..50383a3 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1123,7 +1123,7 @@ static void conf_open_files(struct ctx *c)
>  	if (c->mode == MODE_PASST && c->fd_tap == -1)
>  		c->fd_tap_listen = tap_sock_unix_open(c->sock_path);
>  
> -	c->pidfile_fd = pidfile_open(c->pid_file);
> +	c->pidfile_fd = pidfile_open(c->pidfile);
>  }
>  
>  /**
> @@ -1456,12 +1456,12 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  			break;
>  		case 'P':
> -			if (*c->pid_file)
> +			if (*c->pidfile)
>  				die("Multiple --pid options given");
>  
> -			ret = snprintf(c->pid_file, sizeof(c->pid_file), "%s",
> +			ret = snprintf(c->pidfile, sizeof(c->pidfile), "%s",
>  				       optarg);
> -			if (ret <= 0 || ret >= (int)sizeof(c->pid_file))
> +			if (ret <= 0 || ret >= (int)sizeof(c->pidfile))
>  				die("Invalid PID file: %s", optarg);
>  
>  			break;
> diff --git a/passt.h b/passt.h
> index 3e50612..46d073a 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -184,7 +184,7 @@ struct ip6_ctx {
>   * @nofile:		Maximum number of open files (ulimit -n)
>   * @sock_path:		Path for UNIX domain socket
>   * @pcap:		Path for packet capture file
> - * @pid_file:		Path to PID file, empty string if not configured
> + * @pidfile:		Path to PID file, empty string if not configured
>   * @pidfile_fd:		File descriptor for PID file, -1 if none
>   * @pasta_netns_fd:	File descriptor for network namespace in pasta mode
>   * @no_netns_quit:	In pasta mode, don't exit if fs-bound namespace is gone
> @@ -236,7 +236,7 @@ struct ctx {
>  	char sock_path[UNIX_PATH_MAX];
>  	char pcap[PATH_MAX];
>  
> -	char pid_file[PATH_MAX];
> +	char pidfile[PATH_MAX];
>  	int pidfile_fd;
>  
>  	int one_off;

Looks like trivial refactor so:

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

Re: [PATCH 8/8] conf, passt.h: Rename pid_file in struct ctx to pidfile

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Wed, May 22, 2024 at 10:59:11PM +0200, Stefano Brivio wrote:
> We have pidfile_fd now, pid_file_fd would be quite ugly.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]