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 02/10] Split checking for root from dropping root privilege
Date: Fri, 09 Sep 2022 16:33:27 +0200
Message-ID: <20220909163327.1e2dbc57@elisabeth>
In-Reply-To: <20220908035907.1750314-3-david@gibson.dropbear.id.au>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============6427648302608547105=="

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

Just some ridiculous nitpicking on this one:

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

> [...]
>
> +++ b/passt.1
> @@ -104,9 +104,10 @@ terminal, and to both system logger and standard error=
 otherwise.
> =20
>  .TP
>  .BR \-\-runas " " \fIUID\fR|\fIUID:GID\fR|\fILOGIN\fR|\fILOGIN:GROUP\fR
> -If started as root, change to given UID and corresponding group if UID is =
given,
> +Attempt to change to given UID and corresponding group if UID is given,
>  or to given UID and given GID if both are given. Alternatively, login name=
, or
> -login name and group name can be passed.
> +login name and group name can be passed.  This requires privilege (either

I'd change this to "privileges", right?

Also, adding two spaces following a period, as you do, seems to have
some merits:

  https://link.springer.com/article/10.3758/s13414-018-1527-6

  Johnson, R.L., Bui, B. & Schmitt, L.L. Are two spaces better than
  one? The effect of spacing following periods and commas during reading.
  Atten Percept Psychophys 80, 1504=E2=80=931511 (2018)

...but in man pages, nroff might turn that into three or more spaces,
inconsistently, in a justified paragraph.

I'd stick to one. Or change all of them (I almost never use two, so
here it's all single spaces).

> +initial effective UID 0 or CAP_SETUID capability) to work.
>  Default is to change to user \fInobody\fR if started as root.
> =20
>  .TP
> diff --git a/util.c b/util.c
> index b2ccb3d..17595c1 100644
> --- a/util.c
> +++ b/util.c
> @@ -492,7 +492,13 @@ void check_root(uid_t *uid, gid_t *gid)
>  	char buf[BUFSIZ];
>  	int fd;
> =20
> -	if (getuid() && geteuid())
> +	if (!*uid)
> +		*uid =3D geteuid();
> +
> +	if (!*gid)
> +		*gid =3D getegid();
> +
> +	if (*uid)
>  		return;
> =20
>  	if ((fd =3D open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0)
> @@ -524,11 +530,26 @@ void check_root(uid_t *uid, gid_t *gid)
>  		*uid =3D *gid =3D 65534;
>  #endif
>  	}
> +}
> +
> +/**
> + * drop_root() - Switch to given UID and GID

I would add the usual:

 * @uid:	User ID to switch to
 * @gid:	Group ID to switch to

even though it's totally obvious here, in case somebody should ever need
to parse this stuff to produce documentation.

--=20
Stefano


--===============6427648302608547105==--