Re: [RFC PATCH v2 15/41] SUNRPC: name UUID and GUID users to gssd as uid_r=

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] sunrpc: hang and leaked upcall in gss_encode_v1_msg() when uid= is omitted
--

commit ca819aaf8bcc6fad14f1ba4a44164f940c6b1b37
Author: Jan Koester <jan.koester@tuxist.de>

SUNRPC: name UUID and GUID users to gssd as uid_r=

This commit updates RPCSEC_GSS v1 text upcalls to pass non-legacy user IDs
using the uid_r= key and extends gss_pipe_downcall() to accept a
__kernel_uid_r payload following a sentinel uid of -1. It also rejects binary
v0 upcalls for non-legacy user IDs with -EACCES.

> diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
> index 8ddc65e894dab..5e9e2cf4e9cb4 100644
> --- a/net/sunrpc/auth_gss/auth_gss.c
> +++ b/net/sunrpc/auth_gss/auth_gss.c
> @@ -18,6 +18,7 @@
>  #include <linux/sched.h>
>  #include <linux/pagemap.h>
>  #include <linux/sunrpc/clnt.h>
> +#include <linux/uidgid_r.h>
>  #include <linux/sunrpc/auth.h>
>  #include <linux/sunrpc/auth_gss.h>
>  #include <linux/sunrpc/gss_krb5.h>
> @@ -438,10 +439,23 @@ static int gss_encode_v1_msg(struct gss_upcall_msg *gss_msg,
>  	struct gss_api_mech *mech = gss_msg->auth->mech;
>  	char *p = gss_msg->databuf;
>  	size_t buflen = sizeof(gss_msg->databuf);
> +	char idbuf[KID_STR_LEN];
>  	int len;
>  
> -	len = scnprintf(p, buflen, "mech=%s uid=%d", mech->gm_name,
> -			from_kuid_munged(userns, gss_msg->uid));
> +	/*
> +	 * A UUID or GUID user is uid_r= in its text form instead of uid=,
> +	 * which gssd versions that don't know it reject rather than taking
> +	 * an overflow uid. Where gssd can't see the user at all, there is
> +	 * neither, and the upcall fails.
> +	 */
> +	if (uid_is_legacy(gss_msg->uid))
> +		len = scnprintf(p, buflen, "mech=%s uid=%d", mech->gm_name,
> +				from_kuid_munged(userns, gss_msg->uid));
> +	else if (kuid_has_mapping(userns, gss_msg->uid))
> +		len = scnprintf(p, buflen, "mech=%s uid_r=%s", mech->gm_name,
> +				kuid_str(gss_msg->uid, idbuf));
> +	else
> +		len = scnprintf(p, buflen, "mech=%s", mech->gm_name);

[Severity: High]
When gssd reads from the v1 pipe via rpc_pipe_read() -> gss_v1_upcall() ->
gss_encode_v1_msg(), all three branches return 0 and deliver the upcall text
to userspace with msg->errno == 0.

When rpc_pipe_read() then calls gss_pipe_destroy_msg(), gss_msg is only
unhashed from pipe->in_downcall if msg->errno < 0:

net/sunrpc/auth_gss/auth_gss.c:gss_pipe_destroy_msg() {
	...
	if (msg->errno < 0) {
		refcount_inc(&gss_msg->count);
		gss_unhash_msg(gss_msg);
		if (msg->errno == -ETIMEDOUT)
			warn_gssd();
		gss_release_msg(gss_msg);
	}
	gss_release_msg(gss_msg);
}

Because msg->errno is 0, gss_msg stays on pipe->in_downcall waiting for a
downcall. However, gss_pipe_downcall() matches downcalls to pending upcalls on
pipe->in_downcall solely by uid in gss_find_downcall():

net/sunrpc/auth_gss/auth_gss.c:gss_find_downcall() {
	struct gss_upcall_msg *pos;
	list_for_each_entry(pos, &pipe->in_downcall, list) {
		if (!uid_eq(pos->uid, uid))
			continue;
		...
}

When !kuid_has_mapping(userns, gss_msg->uid), gss_encode_v1_msg() emits only
"mech=%s" without uid= or uid_r=, so gssd is given no user id with which to
construct a matching downcall (and in a non-initial user namespace,
make_kuid_r() in gss_pipe_downcall() also rejects the namespace). Likewise, an
older gssd that does not recognize uid_r= cannot write a __kernel_uid_r
downcall that matches the non-legacy pos->uid in gss_find_downcall().

With no matching downcall possible to unhash gss_msg or set
gss_msg->msg.errno, won't callers in gss_create_upcall() sleep forever in the
wait loop:

net/sunrpc/auth_gss/auth_gss.c:gss_create_upcall() {
	...
	for (;;) {
		prepare_to_wait(&gss_msg->waitqueue, &wait, TASK_KILLABLE);
		spin_lock(&pipe->lock);
		if (gss_msg->ctx != NULL || gss_msg->msg.errno < 0) {
			break;
		}
		spin_unlock(&pipe->lock);
		if (fatal_signal_pending(current)) {
			err = -ERESTARTSYS;
			goto out_intr;
		}
		schedule();
	}
}

while leaving gss_msg (along with its references to gss_auth->kref and
sn->pipe_users) stranded on pipe->in_downcall until the pipe is closed,
causing subsequent upcalls for the same user to coalesce onto the dead
gss_msg in gss_add_msg() and hang as well?

>  	buflen -= len;
>  	p += len;
>  	gss_msg->msg.len = len;

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=15