linux-kernel - Re: [PATCH 1/1] psi: Fix uaf issue when psi trigger is destroyed while being polled

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ydz1jAp6RW3t0owj@sol.localdomain>
Date:   Mon, 10 Jan 2022 19:12:12 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Suren Baghdasaryan <surenb@...gle.com>
Cc:     hannes@...xchg.org, torvalds@...ux-foundation.org, tj@...nel.org,
        lizefan.x@...edance.com, mingo@...hat.com, peterz@...radead.org,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, corbet@....net,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        cgroups@...r.kernel.org, kernel-team@...roid.com,
        syzbot+cdb5dd11c97cc532efad@...kaller.appspotmail.com
Subject: Re: [PATCH 1/1] psi: Fix uaf issue when psi trigger is destroyed
 while being polled

On Mon, Jan 10, 2022 at 06:51:38PM -0800, Suren Baghdasaryan wrote:
> With write operation on psi files replacing old trigger with a new one,
> the lifetime of its waitqueue is totally arbitrary. Overwriting an
> existing trigger causes its waitqueue to be freed and pending poll()
> will stumble on trigger->event_wait which was destroyed.
> Fix this by disallowing to redefine an existing psi trigger. If a write
> operation is used on a file descriptor with an already existing psi
> trigger, the operation will fail with EBUSY error.
> Also bypass a check for psi_disabled in the psi_trigger_destroy as the
> flag can be flipped after the trigger is created, leading to a memory
> leak.
> 
> Reported-by: syzbot+cdb5dd11c97cc532efad@...kaller.appspotmail.com
> Analyzed-by: Eric Biggers <ebiggers@...nel.org>
> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>

Please include Fixes and Cc stable tags.

> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index cafb8c114a21..e6878238fb19 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -3642,6 +3642,12 @@ static ssize_t cgroup_pressure_write(struct kernfs_open_file *of, char *buf,
>  	cgroup_get(cgrp);
>  	cgroup_kn_unlock(of->kn);
>  
> +	/* Allow only one trigger per file descriptor */
> +	if (READ_ONCE(ctx->psi.trigger)) {
> +		cgroup_put(cgrp);
> +		return -EBUSY;
> +	}
> +

Doesn't the task have exclusive access to the file at this point?  READ_ONCE()
is only needed instead of a plain load when the field can be concurrently
changed by another thread.

> diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
> index 1652f2bb54b7..882bf62cc247 100644
> --- a/kernel/sched/psi.c
> +++ b/kernel/sched/psi.c
> @@ -1151,7 +1151,6 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
>  	t->event = 0;
>  	t->last_event_time = 0;
>  	init_waitqueue_head(&t->event_wait);
> -	kref_init(&t->refcount);
>  
>  	mutex_lock(&group->trigger_lock);
>  
> @@ -1180,15 +1179,21 @@ struct psi_trigger *psi_trigger_create(struct psi_group *group,
>  	return t;
>  }
>  
> -static void psi_trigger_destroy(struct kref *ref)
> +void psi_trigger_destroy(void **trigger_ptr)
>  {
> -	struct psi_trigger *t = container_of(ref, struct psi_trigger, refcount);
> -	struct psi_group *group = t->group;
> +	struct psi_trigger *t;
> +	struct psi_group *group;
>  	struct task_struct *task_to_destroy = NULL;
>  
> -	if (static_branch_likely(&psi_disabled))
> +	/*
> +	 * We do not check psi_disabled since it might have been disabled after
> +	 * the trigger got created.
> +	 */
> +	t = xchg(trigger_ptr, NULL);
> +	if (!t)
>  		return;

Likewise, doesn't the task have exclusive access to the file at this point?
This is only called during ->release().

And why does this take a pointer to a pointer instead of just the pointer?

> @@ -1305,14 +1289,24 @@ static ssize_t psi_write(struct file *file, const char __user *user_buf,
>  
>  	buf[buf_size - 1] = '\0';
>  
> -	new = psi_trigger_create(&psi_system, buf, nbytes, res);
> -	if (IS_ERR(new))
> -		return PTR_ERR(new);
> -
>  	seq = file->private_data;
> +
>  	/* Take seq->lock to protect seq->private from concurrent writes */
>  	mutex_lock(&seq->lock);
> -	psi_trigger_replace(&seq->private, new);
> +
> +	/* Allow only one trigger per file descriptor */
> +	if (READ_ONCE(seq->private)) {
> +		mutex_unlock(&seq->lock);
> +		return -EBUSY;
> +	}

Likewise, what does this race against that would require the use of READ_ONCE()?

- Eric