Re: [RFC PATCH v2 26/41] proc: show UUID and GUID ids in status, accept them for loginuid

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] proc: off-by-one TMPBUFLEN in proc_loginuid_write() rejects GUID with newline
- [Medium] proc: torn read of 20-byte loginuid in proc_loginuid_read()
--

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

proc: show UUID and GUID ids in status, accept them for loginuid

This commit adds Uid_r, Gid_r, and Groups_r lines to /proc/<pid>/status to
display user and group ids in text form via kuid_str_munged() and
kgid_str_munged(). It also updates /proc/<pid>/loginuid to accept and
display text-formatted uuid and guid ids alongside classic numeric uids.

> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6a39de424f62a..ec3147e7ab05e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1290,20 +1290,26 @@ static const struct file_operations proc_oom_score_adj_operations = {
>  };
>  
>  #ifdef CONFIG_AUDIT
> -#define TMPBUFLEN 11
> +#define TMPBUFLEN KID_STR_LEN
>  static ssize_t proc_loginuid_read(struct file * file, char __user * buf,
>  				  size_t count, loff_t *ppos)
>  {
>  	struct inode * inode = file_inode(file);
>  	struct task_struct *task = get_proc_task(inode);
> +	struct user_namespace *ns = file->f_cred->user_ns;
>  	ssize_t length;
>  	char tmpbuf[TMPBUFLEN];
> +	kuid_t loginuid;
>  
>  	if (!task)
>  		return -ESRCH;
> -	length = scnprintf(tmpbuf, TMPBUFLEN, "%u",
> -			   from_kuid(file->f_cred->user_ns,
> -				     audit_get_loginuid(task)));
> +	loginuid = audit_get_loginuid(task);
> +	/* a UUID or GUID loginuid as text, if @ns can see it */
> +	if (!uid_is_legacy(loginuid) && kuid_has_mapping(ns, loginuid))
> +		length = strlen(kuid_str(loginuid, tmpbuf));
> +	else
> +		length = scnprintf(tmpbuf, TMPBUFLEN, "%u",
> +				   from_kuid(ns, loginuid));

[Severity: Medium]
Can proc_loginuid_read() observe a torn 20-byte kuid_t when reading
task->loginuid concurrently with proc_loginuid_write() ->
audit_set_loginuid()?

In kernel/audit.c:audit_set_loginuid():

	current->sessionid = sessionid;
	current->loginuid = loginuid;

Updating current->loginuid is an unsynchronized 20-byte struct store, and
audit_get_loginuid(task) in proc_loginuid_read() copies task->loginuid from
a potentially running target task without a lock or seqcount retry loop.

When transitioning between a legacy id (such as INVALID_UID) and a uuid or
guid id, a concurrent reader of /proc/<pid>/loginuid could see the new
.type before all 16 bytes of .uuid/.guid are written (or after .legacy is
overwritten when unsetting), causing kuid_str() to format a corrupted uuid
or guid string. Conversely, seeing the old KID_LEGACY .type with the first
4 bytes of the new .uuid/.guid already stored would cause from_kuid() to
return a bogus numeric uid to userspace.

Would a seqcount or lock around task->loginuid be needed here to prevent
torn reads?

>  	put_task_struct(task);
>  	return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
>  }
> @@ -1312,7 +1318,7 @@ static ssize_t proc_loginuid_write(struct file * file, const char __user * buf,
>  				   size_t count, loff_t *ppos)
>  {
>  	struct inode * inode = file_inode(file);
> -	uid_t loginuid;
> +	char tmpbuf[TMPBUFLEN];
>  	kuid_t kloginuid;
>  	int rv;
>  
> @@ -1332,16 +1338,20 @@ static ssize_t proc_loginuid_write(struct file * file, const char __user * buf,
>  		return -EINVAL;
>  	}
>  
> -	rv = kstrtou32_from_user(buf, count, 10, &loginuid);
> -	if (rv < 0)
> -		return rv;
> +	/* a decimal uid, a UUID or a GUID in braces */
> +	if (count >= sizeof(tmpbuf))
> +		return -EINVAL;
> +	if (copy_from_user(tmpbuf, buf, count))
> +		return -EFAULT;
> +	tmpbuf[count] = '\0';
>  
> -	/* is userspace tring to explicitly UNSET the loginuid? */
> -	if (loginuid == AUDIT_UID_UNSET) {
> -		kloginuid = INVALID_UID;
> -	} else {
> -		kloginuid = make_kuid(file->f_cred->user_ns, loginuid);
> -		if (!uid_valid(kloginuid))
> +	kloginuid = make_kuid_str(file->f_cred->user_ns, tmpbuf);

[Severity: Medium]
Does this size check in proc_loginuid_write() reject a guid in braces when
written with a trailing newline?

In include/linux/uidgid.h, KID_STR_LEN is 39, which is the exact size of a
38-character guid in braces ("{xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx}") plus
the terminating nul byte.

Meanwhile, make_kuid_str() -> kid_r_from_str() in kernel/uidgid_r.c is
designed to accept an optional trailing newline before checking the length
against KID_STR_LEN:

	if (len && str[len - 1] == '\n')
		len--;
	if (!len || len >= sizeof(buf))
		return -EINVAL;

Because TMPBUFLEN is defined as KID_STR_LEN (39), writing a 38-byte guid in
braces followed by '\n' passes count == 39 to proc_loginuid_write(), which
hits count >= sizeof(tmpbuf) and returns -EINVAL before reaching
make_kuid_str().

Should tmpbuf in proc_loginuid_write() (or TMPBUFLEN) be sized to
KID_STR_LEN + 1 so that a guid in braces with a trailing newline is
accepted just like decimal uids and uuids with a trailing newline?

> +	if (!uid_valid(kloginuid)) {
> +		u32 loginuid;
> +
> +		/* is userspace tring to explicitly UNSET the loginuid? */
> +		if (kstrtou32(tmpbuf, 10, &loginuid) ||
> +		    loginuid != AUDIT_UID_UNSET)
>  			return -EINVAL;
>  	}
>

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