From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Brivio <sbrivio@redhat.com>
To: passt-dev@passt.top
Subject: Re: [PATCH v2 03/10] Consolidate determination of UID/GID to run as
Date: Fri, 09 Sep 2022 16:33:47 +0200
Message-ID: <20220909163347.788ddcad@elisabeth>
In-Reply-To: <20220908035907.1750314-4-david@gibson.dropbear.id.au>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5374561422546696269=="

--===============5374561422546696269==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Also mere nitpicking on this one:

On Thu,  8 Sep 2022 13:59:00 +1000
David Gibson <david(a)gibson.dropbear.id.au> wrote:

> Currently the logic to work out what UID and GID we will run as is spread
> across conf().  If --runas is specified it's handled in conf_runas(),
> otherwise it's handled by check_root(), which depends on initialization of
> the uid and gid variables by either conf() itself or conf_runas().
>=20
> Make this clearer by putting all the UID and GID logic into a single
> conf_ugid() function.
>=20
> Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au>
> ---
>  conf.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>  util.c | 50 ------------------------------------
>  util.h |  1 -
>  3 files changed, 73 insertions(+), 59 deletions(-)
>=20
> diff --git a/conf.c b/conf.c
> index 545f61d..5c293b5 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1021,6 +1021,70 @@ static int conf_runas(const char *opt, unsigned int =
*uid, unsigned int *gid)
>  #endif /* !GLIBC_NO_STATIC_NSS */
>  }
> =20
> +/**
> + * conf_ugid() - Determine UID and GID to run as
> + * @runas:	--runas option, may be NULL
> + * @uid:	User ID, set on success
> + * @gid:	Group ID, set on success
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid)
> +{
> +	const char root_uid_map[] =3D "         0          0 4294967295";
> +	struct passwd *pw;
> +	char buf[BUFSIZ];
> +	int ret;
> +	int fd;
> +
> +	/* If user has specified --runas, that takes precedence */
> +	if (runas) {
> +		ret =3D conf_runas(runas, uid, gid);
> +		if (ret)
> +			err("Invalid --runas option: %s", runas);
> +		return ret;
> +	}
> +
> +	/* Otherwise default to current user and group.. */
> +	*uid =3D geteuid();
> +	*gid =3D getegid();
> +
> +	/* ..as long as it's not root.. */
> +	if (*uid)
> +		return 0;
> +
> +	/* ..or at least not root in the init namespace.. */
> +	if ((fd =3D open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0)
> +		return 0;
> +
> +	if (read(fd, buf, BUFSIZ) !=3D sizeof(root_uid_map) ||
> +	    strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) {
> +		close(fd);
> +		return 0;
> +	}
> +
> +	close(fd);
> +
> +	/* ..otherwise use nobody:nobody */

I'd change all those comment to use ellipses (...) instead of "..".

I think your comments here help a lot, by the way (and I couldn't find
a better way to check for UID 0 in non-init other than that hack).

--=20
Stefano


--===============5374561422546696269==--