Re: [RFC PATCH v2 19/41] cred: add the *_r variants of the get/set*id() system calls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] cred: setfsuid_r() and setfsgid_r() commit creds before copying old id
--

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

cred: add the *_r variants of the get/set*id() system calls

This patch adds *_r variants of the get/set*id() system calls that accept
and return pointers to __kernel_uid_r so processes can use UUID or GUID
user and group identifiers. It also splits the classic syscall
implementations into argument conversion helpers and shared do_*()
functions operating on kuid_t and kgid_t.

> diff --git a/kernel/sys.c b/kernel/sys.c
> index 35b538ba843c7..fad55732345de 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c

[ ... ]

> @@ -901,40 +933,44 @@ SYSCALL_DEFINE3(getresgid, gid_t __user *, rgidp, gid_t __user *, egidp, gid_t _
>   * whatever uid it wants to). It normally shadows "euid", except when
>   * explicitly set by setfsuid() or for access..
>   */
> -long __sys_setfsuid(uid_t uid)
> +/*
> + * Sets the fsuid to @kuid, unless it is invalid; 0 or -EPERM. The fsuid
> + * before the call goes to @old.
> + */
> +static long do_setfsuid(kuid_t kuid, kuid_t *old_fsuid)
>  {
>  	const struct cred *old;
>  	struct cred *new;
> -	uid_t old_fsuid;
> -	kuid_t kuid;
>  
>  	old = current_cred();
> -	old_fsuid = from_kuid_munged(old->user_ns, old->fsuid);
> +	*old_fsuid = old->fsuid;
>  
> -	kuid = make_kuid(old->user_ns, uid);
> -	if (!uid_valid(kuid))
> -		return old_fsuid;
> +	if (!uid_valid(kuid) || uid_eq(kuid, old->fsuid))
> +		return 0;
>  
>  	new = prepare_creds();
>  	if (!new)
> -		return old_fsuid;
> +		return -ENOMEM;
>  
>  	if (uid_eq(kuid, old->uid)  || uid_eq(kuid, old->euid)  ||
> -	    uid_eq(kuid, old->suid) || uid_eq(kuid, old->fsuid) ||
> +	    uid_eq(kuid, old->suid) ||
>  	    ns_capable_setid(old->user_ns, CAP_SETUID)) {
> -		if (!uid_eq(kuid, old->fsuid)) {
> -			new->fsuid = kuid;
> -			if (security_task_fix_setuid(new, old, LSM_SETID_FS) == 0)
> -				goto change_okay;
> -		}
> +		new->fsuid = kuid;
> +		if (security_task_fix_setuid(new, old, LSM_SETID_FS) == 0)
> +			return commit_creds(new);
>  	}
>  
>  	abort_creds(new);
> -	return old_fsuid;
> +	return -EPERM;
> +}

[ ... ]

> @@ -945,46 +981,214 @@ SYSCALL_DEFINE1(setfsuid, uid_t, uid)
>  /*
>   * Samma på svenska..
>   */
> -long __sys_setfsgid(gid_t gid)
> +/*
> + * Sets the fsgid to @kgid, unless it is invalid; 0 or -EPERM. The fsgid
> + * before the call goes to @old.
> + */
> +static long do_setfsgid(kgid_t kgid, kgid_t *old_fsgid)
>  {
>  	const struct cred *old;
>  	struct cred *new;
> -	gid_t old_fsgid;
> -	kgid_t kgid;
>  
>  	old = current_cred();
> -	old_fsgid = from_kgid_munged(old->user_ns, old->fsgid);
> +	*old_fsgid = old->fsgid;
>  
> -	kgid = make_kgid(old->user_ns, gid);
> -	if (!gid_valid(kgid))
> -		return old_fsgid;
> +	if (!gid_valid(kgid) || gid_eq(kgid, old->fsgid))
> +		return 0;
>  
>  	new = prepare_creds();
>  	if (!new)
> -		return old_fsgid;
> +		return -ENOMEM;
>  
>  	if (gid_eq(kgid, old->gid)  || gid_eq(kgid, old->egid)  ||
> -	    gid_eq(kgid, old->sgid) || gid_eq(kgid, old->fsgid) ||
> +	    gid_eq(kgid, old->sgid) ||
>  	    ns_capable_setid(old->user_ns, CAP_SETGID)) {
> -		if (!gid_eq(kgid, old->fsgid)) {
> -			new->fsgid = kgid;
> -			if (security_task_fix_setgid(new,old,LSM_SETID_FS) == 0)
> -				goto change_okay;
> -		}
> +		new->fsgid = kgid;
> +		if (security_task_fix_setgid(new, old, LSM_SETID_FS) == 0)
> +			return commit_creds(new);
>  	}
>  
>  	abort_creds(new);
> -	return old_fsgid;
> +	return -EPERM;
> +}

[ ... ]

> +/*
> + * Unlike setfsuid(), which returns the old fsuid whatever happens,
> + * setfsuid_r() reports failure: 0, or -EPERM if the caller may not use
> + * @uid. The fsuid before the call goes to @old_uid unless it is NULL; a
> + * NULL @uid only queries it.
> + */
> +SYSCALL_DEFINE2(setfsuid_r, const struct __kernel_uid_r __user *, uid,
> +		struct __kernel_uid_r __user *, old_uid)
> +{
> +	struct user_namespace *ns = current_user_ns();
> +	kuid_t kuid, old_fsuid;
> +	int err;
> +
> +	err = get_kuid_r(ns, uid, &kuid);
> +	if (err)
> +		return err;
> +	err = do_setfsuid(kuid, &old_fsuid);
> +	if (!err && old_uid)
> +		err = put_kuid_r(ns, old_fsuid, old_uid);
> +	return err;
> +}
> +
> +SYSCALL_DEFINE2(setfsgid_r, const struct __kernel_uid_r __user *, gid,
> +		struct __kernel_uid_r __user *, old_gid)
> +{
> +	struct user_namespace *ns = current_user_ns();
> +	kgid_t kgid, old_fsgid;
> +	int err;
> +
> +	err = get_kgid_r(ns, gid, &kgid);
> +	if (err)
> +		return err;
> +	err = do_setfsgid(kgid, &old_fsgid);
> +	if (!err && old_gid)
> +		err = put_kgid_r(ns, old_fsgid, old_gid);
> +	return err;
> +}

[Severity: Medium]
In kernel/sys.c, if a caller invokes setfsuid_r() or setfsgid_r() with a
valid, permitted uid or gid that differs from the current fsuid or fsgid and
passes a non-NULL old_uid or old_gid pointer that faults during
copy_to_user(), will the syscall return -EFAULT after the new credentials
have already been irreversibly installed?

Specifically, setfsuid_r() calls do_setfsuid(), which updates new->fsuid,
runs security_task_fix_setuid(new, old, LSM_SETID_FS), and calls
commit_creds(new) before put_kuid_r() copies old_fsuid to old_uid (and
similarly for setfsgid_r(), do_setfsgid(), and put_kgid_r()).

If copy_to_user() inside put_kuid_r() or put_kgid_r() fails, the task is left
with its mutated fsuid or fsgid, altered filesystem capabilities, updated
dumpability, and updated thread keyrings even though the syscall reports
failure and the previous ID is lost. Could old_fsuid and old_fsgid be copied
to userspace before committing the new credentials?

[ ... ]

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